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: add ignored annotations #1485

Closed
wants to merge 2 commits into from
Closed

feat: add ignored annotations #1485

wants to merge 2 commits into from

Conversation

moshloop
Copy link
Contributor

@moshloop moshloop commented May 5, 2021

In a slightly different approach to #1482 This adds a new configuration parameter that allows ignoring annotations across services, stateful sets, and pods

pkg/util/config/config.go Outdated Show resolved Hide resolved
Co-authored-by: Felix Kunde <felix-kunde@gmx.de>
@FxKu FxKu added this to the 1.7 milestone May 14, 2021
@FxKu
Copy link
Member

FxKu commented May 31, 2021

@moshloop looks good overall. However, I do not see why it is necessary to move things around. Comparing two K8s services seems fine under k8sutil. You can also move your new unit test there. Then this PR would be only about the new feature and easier to review.

@@ -36,6 +38,7 @@ func init() {
flag.BoolVar(&config.NoTeamsAPI, "noteamsapi", false, "Disable all access to the teams API")
flag.Parse()

config.IgnoredAnnotations = strings.Split(os.Getenv("IGNORED_ANNOTATIONS"), ",")
Copy link
Member

Choose a reason for hiding this comment

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

Can be removed. Operator configuration should happen via ConfigMap or CRD. The other options here are for the operator deployment object.

@FxKu
Copy link
Member

FxKu commented Jun 4, 2021

@moshloop can you rebase with the master and reflect my last comment? I would also appreciate coverage in unit and e2e tests. Should be quite easy to add ignored_annotations to the test_statefulset_annotation_propagation e2e test.

@moshloop
Copy link
Contributor Author

moshloop commented Jun 4, 2021

@FxKu will do

@FxKu
Copy link
Member

FxKu commented Aug 16, 2021

any update here @moshloop ? Was thinking, if we can include it in the next release.

@moshloop
Copy link
Contributor Author

@FxKu Sorry I am a little swamped at the moment I can probably pick this up in a week or 2.

@FxKu
Copy link
Member

FxKu commented Oct 14, 2021

@moshloop any update? Seems like other users / frameworks need this feature.

@FxKu
Copy link
Member

FxKu commented Nov 16, 2021

@moshloop ping?

@moshloop
Copy link
Contributor Author

@FxKu Sorry, I am still snowed under, I am going to try to get someone else on my team to pick this up

@FxKu
Copy link
Member

FxKu commented Mar 24, 2022

@moshloop have now finalized your work in #1823. Thanks again for your contribution.

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

2 participants