-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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 inconsistency in kubernetes commands / arguments handling between jvm and native #27733
Conversation
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.
Spotted some typos in variable names. Use your IDE to fix them as I only suggested the first occurrence.
...oyment/src/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftProcessor.java
Outdated
Show resolved
Hide resolved
...oyment/src/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftProcessor.java
Outdated
Show resolved
Hide resolved
422b0a7
to
4c213c8
Compare
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.
Other necessary changes could be to remove the OpenshiftConfig.getEffectiveJvmArguments
method which is confusing and does not add much value.
Also, we have this getEffectiveJvmArguments
method in JIBConfig and S2IConfig because we had a default value for the jvmArguments, so had two properties jvmArguments
and jvmAdditionalArguments
. We should validate that this default value in jvmArguments
and the jvmAdditionalArguments
are respected after these changes.
Finally, I'm missing some tests to verify the actual change. As far I understand, before these changes, we only generated a command if we didn't know about the base image. Now, we're always generating the command if a custom jar path or the arguments are set by the user, regardless the base image we're using.
Another last note, this could be a breaking change that should be noted in the release notes.
...oyment/src/main/java/io/quarkus/container/image/openshift/deployment/OpenshiftProcessor.java
Outdated
Show resolved
Hide resolved
4c213c8
to
2642001
Compare
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.
The changes look good to me.
Moreover, I checked the reproducer in the linked issue and now the generated DeploymentConfig
resource is the same for JVM and Native. So, the app should behave the same.
The pull request addresses the inconsitencies between jvm and native mode when it comes to command / arguments handling.
With this change the s2i builder image is no longer taken into consideration.
The command may still be
generated
if the user provides arguments without command.Resolves: #24885