🌱 replace exec with client create/update #10442
Conversation
|
Skipping CI for Draft Pull Request. |
k8s-ci-robot
added
do-not-merge/work-in-progress
cahillsf
changed the title
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
/test help |
|
@cahillsf: The specified target(s) for
The following commands are available to trigger optional jobs:
Use
In response to this:
Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. |
|
/test pull-cluster-api-build-main |
|
/test pull-cluster-api-e2e-main |
k8s-ci-robot
added
size/M
|
/test pull-cluster-api-e2e-main |
|
/test pull-cluster-api-e2e-main |
k8s-ci-robot
added
size/L
|
/test pull-cluster-api-e2e-main |
cahillsf
changed the title
k8s-ci-robot
removed
the
do-not-merge/work-in-progress
| if labelSelector.Matches(labels) { | ||
| if err := p.GetClient().Get(ctx, objectKey, existingObject); err != nil { | ||
| // Expected error -- if the object does not exist, create it | ||
| if strings.Contains(err.Error(), "not found") { |
There was a problem hiding this comment.
Is it possible to use apierrors.IsNotFound(err) from apierrors "k8s.io/apimachinery/pkg/api/errors" instead of string comparison?
There was a problem hiding this comment.
Basically we could do something like
if !apierrors.IsNotFound(err) {
return retErrs = append(retErrs, err)
continue
}
if err := p.GetClient().Create(ctx, &o); err != nil {
retErrs = append(retErrs, err)
}| return nil | ||
| existingObject := &unstructured.Unstructured{} | ||
| for _, o := range objs { | ||
| o := o |
There was a problem hiding this comment.
Not needed anymore in go 1.22
| o := o |
There was a problem hiding this comment.
i thought so, but this kept getting flagged in the CI as an issue... struggling to find the results, will remove when i take into account your other feedback and see if it occurs again
There was a problem hiding this comment.
ah then the linter fix is not yet merged I guess (sorry), fine to just make the linter happy 👍
| @@ -358,7 +358,7 @@ func ApplyClusterTemplateAndWait(ctx context.Context, input ApplyClusterTemplate | |||
| WaitForControlPlaneIntervals: input.WaitForControlPlaneIntervals, | |||
| WaitForMachineDeployments: input.WaitForMachineDeployments, | |||
| WaitForMachinePools: input.WaitForMachinePools, | |||
| Args: input.Args, | |||
| LabelSelectors: input.Args, | |||
There was a problem hiding this comment.
we should rename input.Args too
| if errors.As(err, &exitErr) { | ||
| return pkgerrors.New(fmt.Sprintf("%s: stderr: %s", err.Error(), exitErr.Stderr)) | ||
| // Creates or updates objects using the clusterProxy client. | ||
| func (p *clusterProxy) CreateOrUpdate(ctx context.Context, resources []byte, selector ...string) error { |
There was a problem hiding this comment.
being able to provide selector ...string but only at the end allowing to pass a single one just to make the arg optional looks a bit weird and unclean to me.
We could do it similar to GetWorkloadCluster and be able to pass in some opts ...CreateOrUpdateOption, which would also make it somewhat extendable if required in future.
There was a problem hiding this comment.
agreed, i wasnt too happy with this -- sounds good
|
And thanks for tackling this! |
Which issue(s) this PR fixes:
Fixes #9696
/area ci