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

fix: check the value of the annotation to actually create the sidecar #1211

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

dnitsch
Copy link

@dnitsch dnitsch commented Dec 18, 2023

Reason for Change:

#1210

Ensures annotation values are respected

Requirements

  • squashed commits
  • included documentation
  • added unit tests and e2e tests (if applicable).

Issue Fixed:

Please answer the following questions with yes/no:

Does this change contain code from or inspired by another project? If so, did you notify the maintainers and provide attribution?

  • yes
  • no

Notes for Reviewers:

@enj
Copy link
Member

enj commented Dec 18, 2023

This is a breaking change since the existing behavior only requires that the annotation be set at all, not the specific value it has. At most, we can update the documentation to describe the behavior.

@dnitsch
Copy link
Author

dnitsch commented Dec 18, 2023

hi @enj, thanks for coming back on this.

Sure that makes sense and a doc update would be good (happy to PR that, if you could send me the link to the source repo)

Though, the object validation would fail if there is no (string) value for a given key on apply, right?

Possibly, adding an empty "" to the positiveAnnotationVal slice to cover this scenario.

Again I totally see where you are coming from about the breaking change - it's just that I can't see many people setting that value to false and actually expect a sidecar to be injected 😄.

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.

2 participants