fix(feature): uses server-side apply to reconile #1098
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
bartoszmajsak
requested review from
aslakknutsen,
zdtsw,
VaishnaviHire and
cam-garrison
Aligns reconcile strategy for Managed() features to rely on Server-side apply rather than Update. Former apprach is used by deploy package when reconciling kustomize resources. Later tries to overwrite the entire resource rather than using patch which can lead to errors when trying to overwrite immutable fields such as: ``` failed to update object test-namespace/knative-local-gateway: Service "knative-local-gateway" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.clusterIP: Invalid value: "": field is immutable] ``` Additional changes - if "managed" annotation is present in one of the resources being part of the feature it will be respected - reworked tests to be more self-descriptive. extracted funcs were adding more hops while reading the code before understanding what is actually happening
bartoszmajsak
force-pushed
the
server-side-apply
branch
from
ca19202
to
ac34a3d
Compare
zdtsw
reviewed
Jul 4, 2024
| if errJSON != nil { | ||
| return fmt.Errorf("error converting yaml to json: %w", errJSON) | ||
| } | ||
| return cli.Patch(ctx, target, client.RawPatch(k8stypes.ApplyPatchType, data), client.ForceOwnership, client.FieldOwner("rhods-operator")) |
There was a problem hiding this comment.
Took it from the existing trusted bundle one. As I don't pass any owner here (like with deploy/kustomize) I thought using the same should be fine. The naming is outdated, I agree, but if we want to change it, let's change in both places in other PR.
zdtsw
approved these changes
Jul 4, 2024
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: zdtsw The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
openshift-merge-bot
bot
merged commit 9b68ea8
into
opendatahub-io:incubation
8 checks passed
bartoszmajsak
added a commit
to bartoszmajsak/opendatahub-operator
that referenced
this pull request
Jul 8, 2024
Ensures each test case deploys resources in their own namespaces to avoid sharing state between tests (sometimes unintentionally). Follow up to opendatahub-io#1098
5 tasks
openshift-merge-bot bot
pushed a commit
that referenced
this pull request
Jul 8, 2024
Ensures each test case deploys resources in their own namespaces to avoid sharing state between tests (sometimes unintentionally). Follow up to #1098
VaishnaviHire
pushed a commit
to VaishnaviHire/opendatahub-operator
that referenced
this pull request
Jul 24, 2024
Aligns reconcile strategy for Managed() features to rely on Server-side apply rather than Update. Former apprach is used by deploy package when reconciling kustomize resources. Later tries to overwrite the entire resource rather than using patch which can lead to errors when trying to overwrite immutable fields such as: ``` failed to update object test-namespace/knative-local-gateway: Service "knative-local-gateway" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.clusterIP: Invalid value: "": field is immutable] ``` Additional changes - if "managed" annotation is present in one of the resources being part of the feature it will be respected - reworked tests to be more self-descriptive. extracted funcs were adding more hops while reading the code before understanding what is actually happening (cherry picked from commit 9b68ea8)
VaishnaviHire
pushed a commit
to VaishnaviHire/opendatahub-operator
that referenced
this pull request
Jul 24, 2024
Ensures each test case deploys resources in their own namespaces to avoid sharing state between tests (sometimes unintentionally). Follow up to opendatahub-io#1098 (cherry picked from commit 67309b6)
VaishnaviHire
pushed a commit
to VaishnaviHire/opendatahub-operator
that referenced
this pull request
Jul 24, 2024
Aligns reconcile strategy for Managed() features to rely on Server-side apply rather than Update. Former apprach is used by deploy package when reconciling kustomize resources. Later tries to overwrite the entire resource rather than using patch which can lead to errors when trying to overwrite immutable fields such as: ``` failed to update object test-namespace/knative-local-gateway: Service "knative-local-gateway" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.clusterIP: Invalid value: "": field is immutable] ``` Additional changes - if "managed" annotation is present in one of the resources being part of the feature it will be respected - reworked tests to be more self-descriptive. extracted funcs were adding more hops while reading the code before understanding what is actually happening (cherry picked from commit 9b68ea8)
VaishnaviHire
pushed a commit
to VaishnaviHire/opendatahub-operator
that referenced
this pull request
Jul 24, 2024
Ensures each test case deploys resources in their own namespaces to avoid sharing state between tests (sometimes unintentionally). Follow up to opendatahub-io#1098 (cherry picked from commit 67309b6)
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Aligns reconcile strategy for
Managed()features to rely on Server-side apply rather than Update. The SSA approach is used bydeploypackage when reconciling kustomize resources, so keeping it the same makes sense.The previous one, based on
client.Updatecall, tries to overwrite the entire resource rather than using a patch operation, which can lead to errors when trying to overwrite immutable fields such as:Additional changes
How Has This Been Tested?
In
makewe trust.Screenshot or short clip
Merge criteria