-
Notifications
You must be signed in to change notification settings - Fork 104
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
Support overwrite values from existing init-containers #1196
Conversation
e3282c4
to
f9f8b41
Compare
f9f8b41
to
1793a72
Compare
Kudos, SonarCloud Quality Gate passed! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's clearly an improvement over what we have.
I'd like the following:
- apply the same principal for sidecars too.
- change to a more real world example in the integration test.
} | ||
|
||
if (resource.getWorkingDir() != null) { | ||
matching.withWorkingDir(resource.getWorkingDir()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Most String values in the annotation default to empty String. I don't recall by heart how empty Strings translate in the generated Pojos. This highlights an issue:
- The correctness of the this code depends on
what sundrio does for generating Pojos
.
Looking deeper I think that the correctness also depends on:
- Kubernetes model default values.
- ContainerAdapter logic.
This is why I am always a little bit sceptic when it comes to decorators that have such a large surface area.
Given, that this is clearly an improvement over what we have and lacking a better proposal I'll let it pass, but we need to be aware of the dangers in case it backfires.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, I see your point and I agree with you about how dangerous/wrong this code could be. The root cause is that annotations default strings to empty and the properties default to null :/
tests/feat-overwrite-init-containers/src/main/resources/application.properties
Show resolved
Hide resolved
I will do this in a separate pull request after this one is merged if you don't mind. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
Related to quarkusio/quarkus#33097 (comment)