Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

OCI HelmRepo: handle status conditions in-line #748

Merged
merged 2 commits into from
May 31, 2022

Conversation

darkowlzz
Copy link
Contributor

Refactor the OCI HelmRepo reconciler to remove extra custom status
conditions and manage Ready, Reconciling and Stalled conditions within
the reconciler, in-line.
Since OCI HelmRepository is a simple reconciler, it'd be better
to not introduce extra conditions and utilize the three base conditions
to represent the status. In order to have consistent status conditions,
a new summarization code is written within the reconciler based
on the context. It takes into consideration a lot of the details from
the internal/reconcile/summarize package and handles certain scenarios
in context specific manner. All the result and error abstractions are
removed since they are only needed when using internal/reconcile
package.

Some of the code patterns introduced in this can be moved into separate
functions and tested individually, similar to the helpers in the internal/reconcile/
package. It can also be integrated into the summarize and patch helpers.
It can be done separately as a follow-up.

Refer https://gist.github.com/darkowlzz/c5c86afb148ad06dc7c4cc8c6afcdaef
for examples of how the status conditions appear in various scenarios.

Examples of the events:
oci-helmrepo-events
NOTE: Some of the strings may be different in the code. This snapshot was taken during development.

Supersedes #733

darkowlzz and others added 2 commits May 31, 2022 03:53
Refactor the OCI HelmRepo reconciler to remove extra custom status
conditions and manage Ready, Reconciling and Stalled conditions within
the reconciler, in-line.
The internal/reconcile/summarize package uses the patch helper
conditions summary before patching which results in overwriting the
Ready condition with Reconciling condition as it's a negative polarity
condition.
For OCI HelmRepository, since it's a simple reconciler, it'd be better
to not introduce extra conditions and utilize the three base conditions
to represent the status. In order to have the same consistent status
conditions, a new summarization is written within the reconciler based
on the context. It takes into consideration a lot of the details from
the internal/reconcile/summarize package and handles certain scenarios
in context specific ways. All the result and error abstractions are
removed since they are only needed when using internal/reconcile
package.

Signed-off-by: Sunny <darkowlzz@protonmail.com>
Remove stale condition from HelmRepo during garbage collection when a
type switch to OCI HelmRepo occurs. This ensures the OCI HelmRepo does
not have any conditions from the previous type.

Co-authored-by: Soule BA <soule@weave.works>
Signed-off-by: Sunny <darkowlzz@protonmail.com>
@darkowlzz darkowlzz added area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests labels May 30, 2022
@stefanprodan stefanprodan merged commit fe31ff9 into main May 31, 2022
@stefanprodan stefanprodan deleted the oci-helmrepo-refactor branch May 31, 2022 08:30
}
conditions.Delete(obj, meta.StalledCondition)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we already delete the StalledCondition after checking if we have an OCI url, do we need to do it once more?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Based on the context, no, it's not needed.
I added it because of the habit of ensuring a condition is deleted on success when a condition is added on failure.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/helm Helm related issues and pull requests area/oci OCI related issues and pull requests
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants