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

feat: configure excluded propagate labels/annotations #142

Merged

Conversation

erikgb
Copy link
Contributor

@erikgb erikgb commented Jun 30, 2024

This PR proposes an implementation of #53. I have tried to keep changes to a minimum though it seems like the code could benefit from some simple refactorings after this change.

I struggled to find good names for the new config props, so please let me know if you have better suggestions!

The change is theoretically not 100% backwards compatible, as strings.Contains(k, "kubernetes.io/") will potentially exclude more than path.Match("*kubernetes.io/*", key), but IMO it should give the same results for typical labels, at least https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/.

Closes #53

@erikgb erikgb force-pushed the propagate-exclude-labels-annotations branch 2 times, most recently from 2040d4b to 77b97db Compare June 30, 2024 08:52
@erikgb erikgb force-pushed the propagate-exclude-labels-annotations branch from 77b97db to 8829a67 Compare June 30, 2024 09:09
@erikgb erikgb marked this pull request as ready for review June 30, 2024 10:07
@rkwmt rkwmt requested a review from zoetrope July 1, 2024 01:06
Copy link
Member

@zoetrope zoetrope left a comment

Choose a reason for hiding this comment

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

I struggled to find good names for the new config props, so please let me know if you have better suggestions!

I think the current name works fine, but how about simplifying it to excludeLabelKeys, excludeAnnotationKeys?

The change is theoretically not 100% backwards compatible, as strings.Contains(k, "kubernetes.io/") will potentially exclude more than path.Match("kubernetes.io/", key), but IMO it should give the same results for typical labels, at least https://kubernetes.io/docs/concepts/overview/working-with-objects/common-labels/.

No problem at all.

@erikgb
Copy link
Contributor Author

erikgb commented Jul 5, 2024

I struggled to find good names for the new config props, so please let me know if you have better suggestions!

I think the current name works fine, but how about simplifying it to excludeLabelKeys, excludeAnnotationKeys?

I looked in that direction, but I fear some confusion with the existing labelKeys/annotationKeys which denotes labels/annotations on namespaces - which is a quite different configuration. Maybe @ymmt2005 has some input here?

@ymmt2005
Copy link
Member

ymmt2005 commented Jul 5, 2024

@erikgb @zoetrope
Thank you for fixing the issue.

I agree that exclucdeLabelKeys would be confusing, and I'm fine with Eric's suggestion.

@zoetrope
Copy link
Member

zoetrope commented Jul 5, 2024

@erikgb @ymmt2005
Alright, let's merge it as is.

@zoetrope zoetrope merged commit 8ccf25a into cybozu-go:main Jul 5, 2024
8 checks passed
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.

Exclude particular labels/annotations from propagation
3 participants