-
Notifications
You must be signed in to change notification settings - Fork 30
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 wrong ANSI mode option name #420
Conversation
This commit also: - Enforces that the value for --log.ansiMode is valid; - Adds missing support for --dsbulk.log.ansiMode; - Adds missing support for --dsbulk.connector.name; - Adds unit tests for AnsiConfigurator.
@jnorcross would you be kind enough to try this PR and check if it solves your problems? Thanks! |
Looks like the change will fix the documentation issue and correctly validate the parameters. We worked around this aspect by using the one valid option that was available. The change doesn't look like it makes any attempt to change the way the force mode works so I don't think we can add a negative test to our solution to validate that ansi characters appear when using force mode for dsbulk in a C# integration test. We noticed that ansi characters only appear when running our C# code on linux when using normal mode. Force mode made no difference and 'disabled' mode correctly removes them from the linux machines where we saw the issue. |
I'm afraid I cannot help. This doesn't seem to be a problem in dsbulk afaict. I did the below test using this branch: java -jar dsbulk-1.10.0-SNAPSHOT.jar unload --log.ansiMode force And I observed that the colors were present. Then I forced a non-tty stderr by doing: java -jar dsbulk-1.10.0-SNAPSHOT.jar unload --log.ansiMode force 2> log1.txt Here are the contents of log1.txt:
As you can see, Jansi forced the inclusion of ANSI color escape sequences. Then I switched to: java -jar dsbulk-1.10.0-SNAPSHOT.jar unload --log.ansiMode normal 2> log1.txt And I observed that log1.txt did not contain any ANSI escape sequence, which means that Jansi correctly guessed that stderr wasn't a tty. In conclusion, DSBulk seems to behave as expected, at least with standard terminals. |
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.
ok, no worries; we are good for now with the 'disabled' option working fine. +1
If the force mode becomes a problem we will file a new defect with a working example of the program.
Thank you :-) One question before I merge: there has been pressure for pushing out a 1.10 release, so I was wondering: would you mind if I transfer this ticket from 1.9.1 to 1.10.0? The 1.10 would contain just that and a few dependency upgrades to fix CVE issues. |
sure, np. Any upcoming version is fine. We are using 1.8 with the one working version of the mode: Thanks! |
Fixes #419.
This commit also: