Conversation
bhavani111
force-pushed
the
CEQ-12669
branch
from
4c7a177 to
4c000fb
Compare
| if (QuarkusConfiguration.isQuarkusNative()) { | ||
| if (OpenshiftConfiguration.isOpenshift()) { | ||
| QuarkusConfiguration.setProperty("quarkus.native.container-runtime-options", "--platform=linux/amd64"); | ||
| } else { | ||
| QuarkusConfiguration.setProperty("quarkus.native.container-build", "false"); | ||
| } | ||
| } |
There was a problem hiding this comment.
make the changes directly in the buildApp method - as they belong to building the application. there is a properties map already that contains the properties passed to the build
There was a problem hiding this comment.
And also i'm missing some macOS environment check?
There was a problem hiding this comment.
yes, Sorry added it , i will push the latest changes, I thought macos check is not needed since it worked in linux without these properties quarkus.native.container-build ,quarkus.native.container-runtime-options
There was a problem hiding this comment.
I would ideally not set it for anything else then macOS. We wan't to test with the defaults.
bhavani111
force-pushed
the
CEQ-12669
branch
5 times, most recently
from
bc8a7c1 to
ea190ef
Compare
|
Dependency changes No diff |
| @Override | ||
| public void start() { | ||
| logCounter++; | ||
| getProperties(); |
There was a problem hiding this comment.
Maybe setOpenshiftProperties(properties) taking parameter would be more readable?
| boolean isLocal = !OpenshiftConfiguration.isOpenshift(); | ||
| this.properties.put("quarkus.native.container-build", (isMac && isLocal) ? "false" : "true"); | ||
| if (!isLocal) { | ||
| this.properties.put("quarkus.native.container-runtime-options", "--platform=linux/amd64"); |
There was a problem hiding this comment.
This probably wouldn't need to be here and could be moved to OpenshiftQuarkusApp and then you wouldn't need the check? Or do we want to keep it together here to keep the context @avano ?
Also it should be set only for macOS, if am not mistaken.
There was a problem hiding this comment.
yes it is a good idea to move it to openshift quarkus app properties.
it should not matter what is the platform that runs the test, the target platform should always be linux/amd64
There was a problem hiding this comment.
@avano I know, but i would rather vote for testing with "defaults".
There was a problem hiding this comment.
ok then we can add it only for mac
bhavani111
force-pushed
the
CEQ-12669
branch
3 times, most recently
from
cdb72a3 to
71c338f
Compare
| "quarkus.openshift.service-account", getName())); | ||
| boolean isMac = System.getProperty("os.name").toLowerCase().contains("mac"); | ||
| boolean isLocal = !OpenshiftConfiguration.isOpenshift(); | ||
| this.properties.put("quarkus.native.container-build", (isMac && isLocal) ? "false" : "true"); |
There was a problem hiding this comment.
@bhavani111 This should have been set in the QuarkusApp, that is shared for both Standalone and Kubernetes, isn't it?
There was a problem hiding this comment.
Also you are in OpenshiftQuarkusApp, thus you don't need to check isLocal anymore, as you know you are not local.
bhavani111
force-pushed
the
CEQ-12669
branch
3 times, most recently
from
b32d4b1 to
5db5706
Compare
… local development
3 participants
#1778