search

cybozu-go / accurate

Kubernetes controller for multi-tenancy. It propagates resources between namespaces accurately and allows tenant users to create/delete sub-namespaces.
https://cybozu-go.github.io/accurate/
Apache License 2.0
34 stars 5 forks source link

feat: configure excluded propagate labels/annotations #142

Closed erikgb closed 1 week ago

erikgb commented 2 weeks ago

This PR proposes an implementation of https://github.com/cybozu-go/accurate/issues/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 https://github.com/cybozu-go/accurate/issues/53

erikgb commented 1 week ago

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 commented 1 week ago

@erikgb @zoetrope Thank you for fixing the issue.

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

zoetrope commented 1 week ago

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