[WFCORE-5917] Duplicating -Djboss.server.base.dir if defined in more than one place #5110
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
github-actions
bot
added
the
deps-ok
|
Core - Full Integration Build 11603 outcome was FAILURE using a merge of 53e93cd Failed tests |
yersan
requested changes
May 27, 2022
| @@ -332,6 +332,10 @@ echo "" | |||
| echo "=========================================================================" | |||
| echo "" | |||
|
|
|||
| if [[ ! $SERVER_OPTS =~ -Djboss.server.base.dir && ! $JAVA_OPTS =~ -Djboss.server.base.dir ]]; then | |||
There was a problem hiding this comment.
Notice the shebang is sh not bash, so we cannot use some of the enhanced bash features. Even if your current shell allows it, it is not a POXIS standard, for example, it will fail in dash.
There was a problem hiding this comment.
Oh, I see, I will rework it accordingly, Thanks @yersan
|
@yersan Updated |
|
Core -> WildFly Preview Integration Build 11615 outcome was FAILURE using a merge of c146f92 Failed tests |
|
Core -> WildFly Preview Integration Build 11618 outcome was FAILURE using a merge of c146f92 |
|
Core -> WildFly Preview Integration Build 11619 outcome was FAILURE using a merge of c146f92 Failed tests |
|
Core -> WildFly Preview Integration Build 11627 outcome was FAILURE using a merge of c146f92 Failed tests |
|
Core -> WildFly Preview Integration Build 11630 outcome was FAILURE using a merge of c146f92 Failed tests |
|
/retest |
yersan
requested changes
Jun 6, 2022
| @@ -343,7 +347,6 @@ while true; do | |||
| -mp \""${JBOSS_MODULEPATH}"\" \ | |||
| org.jboss.as.standalone \ | |||
| -Djboss.home.dir=\""$JBOSS_HOME"\" \ | |||
| -Djboss.server.base.dir=\""$JBOSS_BASE_DIR"\" \ | |||
There was a problem hiding this comment.
Hi @lvydra , I would say this approach has some issues.
At the moment, when the user configures the server config dir by using JAVA_OPTS, the final value is assigned to the JBOSS_BASE_DIR, however, with your changes, you are removing this possibility. If the config directory is configured on JAVA_OPTS and is unavailable on SERVER_OPTS, the server launch is incorrect. Indeed the JBOSS_BASE_DIR value is a consolidated one that takes into account other OS, so we need to use this value.
Do you agree?
In general, it doesn't matter if the value is in JAVA_OPTS, the server code only uses the arguments to the main method, so if we get it duplicated there, it is at first glance, harmless.
There was a problem hiding this comment.
I think maybe you could remove it from the SERVER_OPTS after being consolidated it on JBOSS_BASE_DIR
There was a problem hiding this comment.
Hi @yersan, yes I agree, I didn't notice that before. So what if we keep checking, if SERVER_OPTS contains
-Djboss.server.base.dir, if so, we will use SERVER_OPTS as they are, however if not JBOSS_BASE_DIR will be added to SERVER_OPTS and that way, the directory configured on JAVA_OPTS could be eventually passed.
There was a problem hiding this comment.
Hi @lvydra , I think we should prefer the one coming from JBOSS_BASE_DIR instead of the one added at SERVER_OPTS.
The JBOSS_BASE_DIR is a consolidated one and looking at the code we have explicitly added computation to consolidate it, e.g symlinks won't be there and it will use the absolute path.
It is a bit weird because having both as server arguments we will get the latest one but that seems unintentional.
About JAVA_OPTS, it is harmless if it is defined there since the server won't use it. We could remove it from that variable after consolidating the setting on JBOSS_BASE_DIR, but I think we could keep it there since the argument is the server doesn't use it and we don't want to compromise any possible user application that is dealing with the value by using that property. After all, is a user-defined variable and shown as part of the launch, it will be even weirder to not see it there if you have defined it.
yersan
requested changes
Jun 8, 2022
| @@ -343,7 +347,6 @@ while true; do | |||
| -mp \""${JBOSS_MODULEPATH}"\" \ | |||
| org.jboss.as.standalone \ | |||
| -Djboss.home.dir=\""$JBOSS_HOME"\" \ | |||
| -Djboss.server.base.dir=\""$JBOSS_BASE_DIR"\" \ | |||
There was a problem hiding this comment.
Hi @lvydra , I think we should prefer the one coming from JBOSS_BASE_DIR instead of the one added at SERVER_OPTS.
The JBOSS_BASE_DIR is a consolidated one and looking at the code we have explicitly added computation to consolidate it, e.g symlinks won't be there and it will use the absolute path.
It is a bit weird because having both as server arguments we will get the latest one but that seems unintentional.
About JAVA_OPTS, it is harmless if it is defined there since the server won't use it. We could remove it from that variable after consolidating the setting on JBOSS_BASE_DIR, but I think we could keep it there since the argument is the server doesn't use it and we don't want to compromise any possible user application that is dealing with the value by using that property. After all, is a user-defined variable and shown as part of the launch, it will be even weirder to not see it there if you have defined it.
|
Core -> WildFly Preview Integration Build 11667 outcome was FAILURE using a merge of 07840b9 Failed tests |
|
Hi @yersan, updated. |
yersan
approved these changes
Jun 9, 2022
yersan
added
the
ready-for-merge
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
https://issues.redhat.com/browse/WFCORE-5917