Skip to content

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

Open
dnitsch wants to merge 1 commit into
Azure:mainfrom
dnitsch:bug/1210-migration-sidecar-annotation
Open

fix: check the value of the annotation to actually create the sidecar#1211
dnitsch wants to merge 1 commit into
Azure:mainfrom
dnitsch:bug/1210-migration-sidecar-annotation

Conversation

@dnitsch

@dnitsch dnitsch commented Dec 18, 2023

Copy link
Copy Markdown

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:

@dnitsch dnitsch requested review from aramase and enj as code owners December 18, 2023 09:41
@enj

enj commented Dec 18, 2023

Copy link
Copy Markdown
Member

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

dnitsch commented Dec 18, 2023

Copy link
Copy Markdown
Author

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.

4 participants