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

fixes: rename default config group label, support/fall back to deprecated labels. #231

Merged
merged 3 commits into from
Jan 15, 2024
Merged

fixes: rename default config group label, support/fall back to deprecated labels. #231

merged 3 commits into from
Jan 15, 2024

Conversation

klihub
Copy link
Collaborator

@klihub klihub commented Jan 12, 2024 •
edited
Loading

This patch set

  • changes the default configuration group label to config.nri/group
  • falls back to look for deprecated labels (group.config.nri, resource-policy.nri.io/group) if the configured label is absent
  • allows overriding the default configuration group label on the command line via helm charts

@klihub klihub requested a review from askervin January 12, 2024 11:50
@klihub
config,agent: use 'config.nri/group' label by default.
e6491e4 
To avoid inconveniences with some existing tooling, change the
default configuration group label to 'prefix.domain/key' format.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub
agent: fall back to deprecated group labels.
77aec84 
Fall back to looking for deprecated group labels if the default or
configured one is not found.

Signed-off-by: Krisztian Litkey <[email protected]>
@klihub klihub marked this pull request as ready for review January 12, 2024 12:48
@klihub klihub mentioned this pull request Jan 12, 2024
Merged
marquiz
marquiz reviewed Jan 12, 2024
View reviewed changes
Copy link
Collaborator

@marquiz marquiz left a comment

Choose a reason for hiding this comment

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

I think this is ok in the project in this early stage. But generally I think deprecating things in a patch release are questionable (or other functional changes)

@klihub
Copy link
Collaborator Author

klihub commented Jan 12, 2024 •
edited
Loading

I think this is ok in the project in this early stage. But generally I think deprecating things in a patch release are questionable (or other functional changes)

IMO as long as we keep the deprecated things intact/supported throughout the whole main release cycle where the patch release belongs to (IOW in this case 0.3.x) and if timing requires (IOW deprecation is too close to the end of the main release lifecycle) also during the next one (IOW in our case that would be 0.4.x), then it should be perfectly fine to deprecate things in any patch release.

This patch set should not introduce any functional changes, unless we consider an additional log message a functional change. If things worked for you without this patch set, they should work the same with this patch set in place, too.

askervin
askervin approved these changes Jan 12, 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.

Looks great! Thank you @klihub!

fmuyassarov
fmuyassarov approved these changes Jan 12, 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.

LGTM

@klihub klihub merged commit 7c70158 into containers:main Jan 15, 2024
3 checks passed
@klihub klihub deleted the fixes/config-group-labels branch January 15, 2024 09:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@marquiz marquiz marquiz left review comments

@fmuyassarov fmuyassarov fmuyassarov approved these changes

@askervin askervin askervin approved these changes

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

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