Refactor to adopt k8s standardized Condition type #195

relu · 2020-11-04T19:19:10Z

Updates to use metav1.Condition type and removes references for
deprecated corev1.Condition* constants and uses the new k8s api/meta
helpers in place of the old pkg/apis/meta types.

Relates to fluxcd/flux2#225

NB: This will be a blocker for other components that depend on the source-controller. We'll need to release a new version of the source-controller before proceeding to update the other components that depend on it:

kustomize-controller
helm-controller
notification-controller
image-automation-controller

seaneagan · 2020-11-04T20:05:31Z

api/v1beta1/bucket_types.go

 // SetBucketCondition sets the given condition with the given status, reason and
 // message on the Bucket.
-func SetBucketCondition(bucket *Bucket, conditionType string, status corev1.ConditionStatus, reason, message string) {
+func SetBucketCondition(bucket *Bucket, conditionType string, status metav1.ConditionStatus, reason, message string) {


Can we remove this method (and the corresponding methods for the other types) and just use metav1.SetStatusCondition() instead? That would give us the correct lastTransitionTime handling, and would allow setting the observedGeneration on the passed condition.

Or perhaps use metav1.SetStatusCondition() within these methods. Or if possible add a convenience method to pkg/runtime which has the same signature as these methods, except with a loosely typed first argument and gets the metadata generation and status conditions, and then calls metav1.SetStatusCondition().

Makes sense to me. Will follow-up with the changes.

I think we can also remove FilterOutCondition from fluxcd/pkg now that we have meta.RemoveStatusCondition()?

I think we can also remove FilterOutCondition from fluxcd/pkg now that we have meta.RemoveStatusCondition()?

If that was its intended purpose then sure. From the implementation (and name) it was not that clear, to be honest.

I missed this during my review. But they behave indeed the same, except that the one in fluxcd/pkg returns a new slice and the one from the Kubernetes upstream modifies the given slice. The latter is more Kubernetes idiomatic, and has my preference (we should work with pointers as much as we can actually (that being: when we modify state)).

Here's the removal of FilterOutCondition: fluxcd/pkg#47

Or perhaps use metav1.SetStatusCondition() within these methods. Or if possible add a convenience method to pkg/runtime which has the same signature as these methods, except with a loosely typed first argument and gets the metadata generation and status conditions, and then calls metav1.SetStatusCondition().

I suppose having a dedicated convenience method in pkg would be nice if we actually do want to handle updating the observedGeneration of the conditions as well during.

On a side note, looks like updating the ObservedGeneration in the SetStatusCondition func was added quite recently to v1.20.

Or perhaps use metav1.SetStatusCondition() within these methods. Or if possible add a convenience method to pkg/runtime which has the same signature as these methods, except with a loosely typed first argument and gets the metadata generation and status conditions, and then calls metav1.SetStatusCondition().

~~Implemented here: fluxcd/pkg#48~~
Never mind that, it's not gonna work because there's no common type I can use between the different resource types we use in the apis.

controllers/bucket_controller.go

controllers/gitrepository_controller.go

controllers/helmchart_controller.go

controllers/helmchart_controller_test.go

controllers/helmrepository_controller.go

hiddeco · 2020-11-17T13:21:58Z

⚠️ Your branch needs a rebase for the tests to start.

relu · 2020-11-17T13:48:55Z

Your branch needs a rebase for the tests to start.

Yes, I'm working on that. Already did the suggested changes and I'm working to resolve the conflicts now :)

hiddeco

The changes look good to me, but this needs to be merged at a strategic moment.

Thanks @relu 🌻

relu · 2020-11-17T18:08:34Z

needs to be merged at a strategic moment.

Definitely. I'm going to continue with these changes in the other dependent controllers and submit draft PRs pointing the dependencies to pseudo-versions. After you decide when these changes will go out, they can be merged in order with minimal changes (version bumps for deps basically).

stefanprodan

LGTM

I'm ok with merging this tomorrow and do a release of all controllers

Updates to use metav1.Condition type and removes references for deprecated corev1.Condition* constants and uses the new k8s api/meta helpers in place of the old pkg/apis/meta types. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

Change to rely on the upstream k8s api/meta SetStatusCondition to set/update the resources status conditions rather than FilterOutCondition from apis/meta which is due to be removed. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

Use SetResourceCondition as a generic method to set conditions for CRs, implmeneting the ObjectWithStatusConditions interface used as input type. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

seaneagan suggested changes Nov 4, 2020

View reviewed changes

relu force-pushed the adopt-k8s-conditions branch from 1845434 to ec975d3 Compare November 17, 2020 12:55

hiddeco reviewed Nov 17, 2020

View reviewed changes

relu force-pushed the adopt-k8s-conditions branch 2 times, most recently from 29e5432 to 239b514 Compare November 17, 2020 14:15

hiddeco approved these changes Nov 17, 2020

View reviewed changes

hiddeco requested a review from stefanprodan November 17, 2020 15:42

hiddeco added enhancement New feature or request hold Issues and pull requests put on hold labels Nov 17, 2020

This was referenced Nov 17, 2020

Refactor to adopt k8s standardized Condition type fluxcd/kustomize-controller#174

Merged

Refactor to adopt k8s standardized Condition type fluxcd/notification-controller#83

Merged

Update components to v0.3.0 fluxcd/flux2#474

Merged

stefanprodan approved these changes Nov 18, 2020

View reviewed changes

hiddeco mentioned this pull request Nov 18, 2020

Refactor to adopt k8s standardized Condition type fluxcd/helm-controller#148

Merged

relu added 3 commits November 19, 2020 13:08

Refactor to adopt k8s standardized Condition type

00bb853

Updates to use metav1.Condition type and removes references for deprecated corev1.Condition* constants and uses the new k8s api/meta helpers in place of the old pkg/apis/meta types. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

Use SetStatusCondition vs FilterOutCondition

69f9bbd

Change to rely on the upstream k8s api/meta SetStatusCondition to set/update the resources status conditions rather than FilterOutCondition from apis/meta which is due to be removed. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

Switch to new pkg/apis/meta SetResourceCondition

b80f450

Use SetResourceCondition as a generic method to set conditions for CRs, implmeneting the ObjectWithStatusConditions interface used as input type. Signed-off-by: Aurel Canciu <aurelcanciu@gmail.com>

relu force-pushed the adopt-k8s-conditions branch from 239b514 to b80f450 Compare November 19, 2020 11:08

stefanprodan removed the hold Issues and pull requests put on hold label Nov 19, 2020

stefanprodan merged commit 5533469 into fluxcd:main Nov 19, 2020

relu deleted the adopt-k8s-conditions branch November 19, 2020 12:39

Refactor to adopt k8s standardized Condition type #195

Refactor to adopt k8s standardized Condition type #195

Uh oh!

Conversation

relu commented Nov 4, 2020

Uh oh!

seaneagan Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

seaneagan Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

relu Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

seaneagan Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

relu Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

hiddeco Nov 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

relu Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

relu Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

relu Nov 4, 2020

Choose a reason for hiding this comment

Uh oh!

relu Nov 5, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

hiddeco commented Nov 17, 2020

Uh oh!

relu commented Nov 17, 2020

Uh oh!

hiddeco left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

relu commented Nov 17, 2020

Uh oh!

stefanprodan left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

seaneagan Nov 4, 2020 •

edited

Loading

hiddeco Nov 4, 2020 •

edited

Loading

relu Nov 5, 2020 •

edited

Loading

hiddeco left a comment •

edited

Loading