-
Notifications
You must be signed in to change notification settings - Fork 306
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
FISH-1423 disable escaping asadmin #5299
Conversation
Signed-off-by: Ondro Mihalyi <[email protected]>
…dmin Copy code from my fork to a branch
jenkins test please |
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.
We probably need a swarm on this to test it on all supported OSs and terminals, across a variety of commands, and comparing against the asadmin recorder - escape characters are finicky.
nucleus/admin/cli/src/main/java/com/sun/enterprise/admin/cli/CLICommand.java
Outdated
Show resolved
Hide resolved
nucleus/admin/cli/src/main/java/com/sun/enterprise/admin/cli/CLICommand.java
Outdated
Show resolved
Hide resolved
nucleus/admin/cli/src/main/java/com/sun/enterprise/admin/cli/CLICommand.java
Show resolved
Hide resolved
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.
simplify system property, fix alignment
jenkins test please |
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.
Tabs in the code and 2-space indents still there
@Pandrex247 Since this PR just passes additional option and has ability to disable it, should this not just be good enough approved for community? |
jenkins test |
@lprimak this is targeted to go into Community and Enterprise at the same time - if anthing other than the default value needs changing we need to keep them in sync so doing the tests here first makes sense to save us having to make a 2nd PR. Results of testing on Windows (Windows Terminal):
Results of testing on WSL OpenSUSE Leap 15.2 (Windows Terminal):
@OndroMih Is this in line with what you expect? |
@Pandrex247 You are setting the wrong Environment variable, I recently changed it to AS_ADMIN_DISABLE_EVENT_EXPANSION |
@kalinchan Not in the code pushed to this branch |
@Pandrex247 That Environment class you said to use adds that prefix to the String, so it becomes AS_ADMIN_DISABLE_EVENT_EXPANSION |
Comment updated with new results. |
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, @Pandrex247, the behavior you observe is expected. This issue is only about fixing multimode, command line execution used to work before as expected. The only case that didn't work for you on Windows is actually also expected - Windows doesn't use \
as escape character and therefore it's not needed to escape it as in Linux shell's. We can't change how Windows escapes characters.
I suggested one small cosmetic change (use the existing Environment instead of creating a new one), otherwise all looks good.
nucleus/admin/cli/src/main/java/com/sun/enterprise/admin/cli/CLICommand.java
Outdated
Show resolved
Hide resolved
jenkins test |
jenkins test |
Jenkins test please |
…-asadmin FISH-1423 disable escaping asadmin
…escaping-asadmin" This reverts commit 6327682 Signed-off-by: JamesHillyard <[email protected]>
…-asadmin FISH-1423 disable escaping asadmin
Description
Allows the ability to enable/disable escape event
Testing
Testing Performed
Tested by seeing if this the expected outcome is expected, and then setting the env variable AS_ADMIN_DISABLE_EVENT_EXPANSION to false and seeing if it is different
Testing Environment
openjdk version "1.8.0_292"
ubunutu 20.04
maven 3.6.3
Documentation PR
payara/Payara-Community-Documentation#169
Notes for Reviewers
Started by @OndroMih