fix(feature): only relies on desired state for reconcilation

bartoszmajsak commented 2 months ago

Description

This change ensures that the "management mode" is based exclusively on a resource defined as the desired state, i.e. the one provided by the controller itself. This can be, but is not limited to, manifest files provided through an embedded file system.

Although the actual instance on the cluster (the target) might be in a different state (the user might want to opt-out from reconcilation by changing the label), we intentionally do not address this scenario due to the lack of clear requirements on how users can modify such resources.

This change allows control over previously undefined resources, so those created by Feature API without explicit management state defined. As long as the original Feature does not have Managed() called in the builder chain, nor used manifests have the label explicitly defined the behaviour remains unchanged, which means the resources being part of such a feature are not managed and thus not reconciled after initial apply.

opendatahub.io/managed is already exposed as an annotation and used by other components.

Follow up to #974

How Has This Been Tested?

Integration tests are included in tests/integration/features/resources_int_test.go

Screenshot or short clip

Merge criteria

[x] You have read the contributors guide.
[x] Commit messages are meaningful - have a clear and concise summary and detailed explanation of what was changed and why.
[x] Pull Request contains a description of the solution, a link to the JIRA issue, and to any dependent or related Pull Request.
[ ] ~Testing instructions have been added in the PR body (for PRs involving changes that are not immediately obvious).~
[x] The developer has manually tested the changes and verified that the changes work

bartoszmajsak commented 1 month ago

Putting on hold until label/annotation confusion is addressed

bartoszmajsak commented 1 month ago

Putting on hold until label/annotation confusion is addressed

that has been clarified now - annotation that is.

bartoszmajsak commented 1 month ago

So, this changes from looking at the bundled manifests, rather than looking at the resource in the cluster.

Correct, and note it is not used anywhere in the Features yet (at least in the incubation branch). I have a fix for envoy filter coming which could use this though.

I'm fine with the change, as it is backwards compatible. Although, it should be noted that in the documented case the annotation is editable by the user to disengage reconciliation, while in Features developers have the control. So, it is two different meanings. Just to keep in mind.

This is why I mentioned in the godocs, that the requirements are not clear yet - we only handle it for one specific case for KServe kustomize manifests through deploy.go which got brought back in #1106. Perhaps the docs should mention it is only applicable for this very resource at the moment.

openshift-ci[bot] commented 1 month ago

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: cam-garrison, israel-hdez, 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: - ~~[OWNERS](https://github.com/opendatahub-io/opendatahub-operator/blob/incubation/OWNERS)~~ [zdtsw] Approvers can indicate their approval by writing `/approve` in a comment Approvers can cancel approval by writing `/approve cancel` in a comment

israel-hdez commented 1 month ago

Perhaps the docs should mention it is only applicable for this very resource at the moment.

Here I have to defer to @zdtsw and @VaishnaviHire about the godocs. If you mean about the doc-comments in the code, I also appreciate clarification there for us, the developers. About the other docs mentioned in the main comment, I guess the policy is that only documented uses are supported; so those docs should be fine as is.

opendatahub-io / opendatahub-operator