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

Set default observedGeneration to -1 #277

Merged
merged 1 commit into from
Jan 22, 2022

Conversation

sbernheim
Copy link
Contributor

@sbernheim sbernheim commented Dec 10, 2021

Sets a default value of -1 for the observedGeneration field of the ImageUpdateAutomations type status.observedGeneration attribute. This ensures that tools like kstatus do not consider the resource to be in a Ready state prematurely because the generation and observedGeneration attributes are briefly initialized with 0 values.

Credits to @tomhuang12 for his prior work on fluxcd/image-reflector-controller#189

@sbernheim
Copy link
Contributor Author

Attempting to correct for the failed check now.

@stefanprodan
Copy link
Member

stefanprodan commented Dec 11, 2021

@sbernheim you shouldn’t edit edit the manifests manually as they are generated from code. Please run make manifests.

@sbernheim
Copy link
Contributor Author

l had to do a bunch of things to get the tests to run.

The tests don't seem to want to work without Docker which I was trying to replace with containerd and podman. In the end I gave up on figuring out how to make that work and reinstalled Docker for Desktop on my MacBook again.

I also had to add an .envrc file with this env var export:

export PKG_CONFIG_PATH="/usr/local/Cellar/openssl@1.1/1.1.1m/lib/pkgconfig"

That directory is where brew install openssl@1.1 put the openssl.pc file that needs to be linked for the tests.

Now of course the tests are failing but with a different error message that the GitHub checks which indicates that the default observedGeneration value is supposed to be an integer and not a string. The make manifests command generates the CRD file with the default value set to a string "-1" instead of an integer -1 and I'm not sure how to fix that.

All the long string values in the generated CRD's description fields are also wrapped at 90 characters, which seems like it'd be a linter setting.

However, the same tests fail on my local and the generated CRD has the same problem when I run it against the main branch for the image-reflection-controller project. So I have some sort of version conflict between my environment and yours - probably the wrong kubebuilder version installed on my local. I installed the latest today while setting all this up, and more likely need to drop back to an older version.

I'll dig a bit more and try to resolve these issues over the next week or so.

@sbernheim
Copy link
Contributor Author

Found an open issue for the line-wrapping problem, which has a suggested mitigation in the most recent comment kubernetes-sigs/controller-tools#514

Putting the link here so I don't forget where I saw that later when I have time to try again.

@sbernheim sbernheim force-pushed the default-observed-generation branch 2 times, most recently from ac64b82 to 47c8d56 Compare December 23, 2021 03:19
@sbernheim
Copy link
Contributor Author

Pinning controller-gen to v0.5.0 seems to have resolved the problem of the default value being set as a string in the generated CRD.

go get sigs.k8s.io/controller-tools/cmd/controller-gen@v0.5.0

It's still wrapping all the long text lines at 90 chars, though. I pushed what I have to see if the tests pass, but obviously I'd rather not change all the description fields like this. I'll have to circle back and figure out how to fix that another day.

@makkes
Copy link
Member

makkes commented Jan 5, 2022

@sbernheim I created a commit on my own fork that should fix CI: makkes@28fc2cb

Feel free to cherry-pick it into this PR's branch.

@sbernheim sbernheim force-pushed the default-observed-generation branch 4 times, most recently from 527d1a0 to e36ca0a Compare January 21, 2022 01:15
@sbernheim
Copy link
Contributor Author

@makkes - Thanks for that! I cherry-picked your PR into my branch, but the "dirty tree" check still failed!

It appears that @relu merged a commit to main while updating the Flux pkg components that wrapped text lines at 90 characters that match the ones I'm generating on my local.

Took me a while to figure out why that last round of testing was failing. At first I thought I must be reading the diff backwards and had somehow dropped your commit again after merging it to my branch. I figured out what had happened once I took a little dive into the blame history for the generated CRD file.

Hopefully the tests will succeed this time and we can finally put this one to bed. All's well that ends well AFAIC!

go.sum Outdated Show resolved Hide resolved
go.sum Outdated Show resolved Hide resolved
Copy link
Member

@relu relu left a comment

Choose a reason for hiding this comment

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

LGTM, thank you @sbernheim! ✔️

Sets a default value of -1 for the observedGeneration field of the
ImageUpdateAutomations type status.observedGeneration attribute.
This ensures that tools like kstatus do not consider the resource to be
in a Ready state prematurely because the generation and
observedGeneration attributes are briefly initialized with 0 values.

Signed-off-by: Sebastian Bernheim <sebastian@weave.works>
@relu relu merged commit d53c894 into fluxcd:main Jan 22, 2022
@sbernheim
Copy link
Contributor Author

🎉 !!!

@sbernheim sbernheim deleted the default-observed-generation branch January 22, 2022 00:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants