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.

Sign up for GitHub

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

Jump to bottom

pkg/apis, helm: update balloons config/CRD. #222

Merged
merged 3 commits into from
Jan 12, 2024
Merged

pkg/apis, helm: update balloons config/CRD. #222

merged 3 commits into from
Jan 12, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Jan 9, 2024 •
edited
Loading

It looks like the latest balloons configuration additions have been committed then merged without updating the configuration, CRDs and clientsets. Let's fix it. Also, while at it let's add generated artifact verification to our github verification workflow.

askervin
askervin reviewed Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

Great that you spotted this, @klihub!

It seems that I added preferCloseToDevices only to helm deployent crd in commit 1016427. Should have done more careful grep to find other CRD files to be modified. As explained, preferFarFromDevices was intentionally left out from CRDs for now.

config/crd/bases/config.nri_balloonspolicies.yaml Outdated Show resolved Hide resolved
config/crd/bases/config.nri_balloonspolicies.yaml Outdated Show resolved Hide resolved
@fmuyassarov
Copy link
Collaborator

I wonder if we should extend one of the workflows or introduce a new one to check if all CRDs are re-generated correctly.

@klihub klihub force-pushed the fixes/update-balloons-config branch from 0b257bf to f2c2b8c Compare January 10, 2024 12:24
@klihub
Copy link
Collaborator Author

klihub commented Jan 10, 2024 •
edited
Loading

Great that you spotted this, @klihub!

It seems that I added preferCloseToDevices only to helm deployent crd in commit 1016427. Should have done more careful grep to find other CRD files to be modified. As explained, preferFarFromDevices was intentionally left out from CRDs for now.

All our CRD's are generated and should not be touched by hand. The proper way of updating them is to update the corresponding types somewhere under pkg/apis/config/v1alpha1/** then run make generate. This should generate the CRDs, copy them to the necessary duplicate places and make sure that running make generate after that is idempotent until further similar changes. We'll probably want to add a github verification workflow which just runs make generate and verifies that it does not induce any changes to the repo.

@klihub
Copy link
Collaborator Author

klihub commented Jan 10, 2024

I wonder if we should extend one of the workflows or introduce a new one to check if all CRDs are re-generated correctly.

I had exactly the same idea. I think too that we should.

@klihub klihub force-pushed the fixes/update-balloons-config branch from d3958ab to 291231f Compare January 10, 2024 12:41
@klihub
Copy link
Collaborator Author

klihub commented Jan 10, 2024

I wonder if we should extend one of the workflows or introduce a new one to check if all CRDs are re-generated correctly.

I had exactly the same idea. I think too that we should.

Hooked in new verify-generate to the top level verify target. Now our github CI workflows should verify that all generated artifacts are properly committed and up-to-date in the repo.

@klihub klihub requested a review from askervin January 10, 2024 12:45
@klihub
pkg/apis/config: fix balloons config tags.
2f2cfc9 
Fix `omitempty` JSON tags for balloons config `PreferCloseToDevices`
and `IdleCpuClass`, so they properly imply optionality in the
corresponding CRD fields.

Tag `PreferFarFromDevices` is considered unstable/untested and should
not be used. Hence, tag it as `"-"` to prevent unmarshalling it from
a CRD to the actual configuration.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub
pkg/apis, helm: update balloons config/CRD.
4580950 
It looks like the latest balloons configuration additions have
been committed then merged without updating the configuration,
CRDs and clientsets. Let's fix it.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub
Makefile: add 'verify-generate' target.
aba21af 
Add target for verifying that all generated artifacts are up to
date in the repo. Hook it in to the top-level verify target, so
it will be run as part of our github CI workflows.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub force-pushed the fixes/update-balloons-config branch from 291231f to aba21af Compare January 10, 2024 13:28
fmuyassarov
fmuyassarov approved these changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@fmuyassarov fmuyassarov left a comment

Choose a reason for hiding this comment

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

Thank you @klihub for the fixes.
LGTM

askervin
askervin approved these changes Jan 10, 2024
View reviewed changes
Copy link
Collaborator

@askervin askervin left a comment

Choose a reason for hiding this comment

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

LGTM

@klihub klihub mentioned this pull request Jan 12, 2024
Merged
@klihub
Copy link
Collaborator Author

klihub commented Jan 12, 2024

#230 is a backport of this to the release-0.3 branch.

@klihub klihub merged commit 3478c11 into containers:main Jan 12, 2024
5 checks passed
fmuyassarov added a commit that referenced this pull request Jan 12, 2024
@fmuyassarov
Merge pull request #230 from klihub/backports/release-0.3/update-ball…
cf661c6 
…oons-config

[release-0.3/#222] pkg/apis, helm: update balloons config/CRD.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fmuyassarov fmuyassarov fmuyassarov approved these changes

@askervin askervin askervin approved these changes

@marquiz marquiz Awaiting requested review from marquiz

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@klihub @fmuyassarov @askervin