[ELY-2082] Optimise Tool Help Text #1713

petrberan · 2022-06-03T12:23:46Z

https://issues.redhat.com/browse/ELY-2082

tool/src/main/java/org/wildfly/security/tool/help/CommandsSection.java

Skyllarr · 2022-06-13T09:01:31Z

tool/src/main/java/org/wildfly/security/tool/ElytronToolMessages.java

@@ -607,4 +607,9 @@ public interface ElytronToolMessages extends BasicLogger {
    @Message(id = 35, value = "Only one of '%s' and '%s' can be specified at the same time")
    IllegalArgumentException mutuallyExclusiveOptions(String first, String second);

+    @Message(id = NONE, value = "To get list of options for specific command, please specify the command by using ./elytron-tool.sh [command] --help")


Just a minor, this should be placed before the Numeric Errors comment

s/get list of options for specific command/get a list of options for a specific command

Skyllarr · 2022-06-13T10:08:50Z

@petrberan This looks good! Later I will try on windows also just to make sure it prints out well.

With this formating I noticed that we should standardize our output when defining commands. Currently we use single/double quotes randomly and FileSystemRealm should be filesystem-realm:

credential-store            "credential-store" command is used to perform various operations on credential store.

filesystem-realm            'FileSystemRealm' command is used to convert legacy properties files and scripts to an
                             Elytron FileSystemRealm.

petrberan · 2022-06-13T13:42:55Z

Thank you @Skyllarr for the Windows testing.

As for the single/double quotes, in the recent forced commit I unified the quotes to be double ones, as I believe all commands are represented by strings rather than characters.

By looking at those messages I believe we could also go and polish the rest as well.

There are some descriptions that are missing the full stop:

@Message(id = NONE, value = "The alias of the secret key stored in the credential store file. Set to key by default") String cmdFileSystemEncryptSecretKeyDesc();

Inconsistency in the naming conventions, such as credential-store vs credential store vs Credential Store
And perhaps even the "cause of action" in several options, if you want, such as --aliases: changing Display all aliases to Displays all aliases. It may be only me, but I feel the difference between "You want the option to display all aliases" and "The option displays all aliases"

Skyllarr · 2022-06-27T14:17:07Z

@petrberan Thank you for unifying the quotes! As for windows testing, when running the tool from the windows command line I get the following:

java -jar
target\wildfly-elytron-tool-shaded-1.19.1.CR1-SNAPSHOT.jar

←[1;96mDESCRIPTION←[0m

    A tool that assists with Elytron configuration

←[1;96mUSAGE←[0m

    ./elytron-tool.sh [command] [options]

←[1;96mCOMMANDS←[0m

so the ANSI escape codes do not work on windows it seems

Skyllarr · 2022-06-27T14:43:38Z

@petrberan I did not find yet, but there could be some library/specific codes that can be used to have working colors in both environments. Or in the worst case we can do without colors

petrberan · 2022-06-28T11:24:34Z

Thank you @Skyllarr for looking into this. I was afraid that it wouldn't work on Windows, apparently it used to work before Win 10 or something like that. Regardless, there are multiple way how to tackle this problem:

Completely ditch colored headers
Use Aesh library which WFCORE already uses for the Jboss CLI
Go for a different library

Personally, I'd go for option 2) since I believe CLI colors on Windows work fine. If for some reason it would not be possible to use Aesh, I'd find a different library (3), something lightweight preferably. And if all that fails, then we'd have to go without fancy colors (1).

Skyllarr · 2022-06-28T15:17:36Z

@petrberan Thank you for explanation! Yes I agree with your idea of trying what wilfly-core uses first and then see if anything else needs to be done

petrberan · 2022-07-19T12:55:50Z

@Skyllarr I managed to use the Aesh library from WFCORE, just two lines here so the Apache CLI functionality is intact. The color works just fine on my machine, could you please try it on Windows as well?

You need to add <module name="org.aesh" export="true"/> to this module.xml in WFCORE. If the text would be fine and these changes approved, I will create a WFCORE issue for adding the dependency, same way as for this issue

Skyllarr · 2022-10-25T14:25:55Z

tool/src/main/java/org/wildfly/security/tool/help/CommandsSection.java

+            stringBuilder.append(System.lineSeparator());
+        }
+
+        else{


Just a minor, this could be formatted better

Skyllarr · 2022-10-25T15:39:45Z

@petrberan I really apologize for late review. I tried on windows and colors are working great! There is 1 issue I noticed, see the included picture. Basically when the terminal is narrow, there are some misaligned new lines in the options sections. This is happening on linux as well. Can you tell where the problem could be? The picture is result of ./elytron-tool.sh credential-store -h in narrow terminal

UPDATE: I tried mvn -h and the same is happening for maven help as well, so this is not a problem, nevermind this comment

Skyllarr · 2022-10-25T16:17:09Z

@Skyllarr I managed to use the Aesh library from WFCORE, just two lines here so the Apache CLI functionality is intact. The color works just fine on my machine, could you please try it on Windows as well?

You need to add <module name="org.aesh" export="true"/> to this module.xml in WFCORE. If the text would be fine and these changes approved, I will create a WFCORE issue for adding the dependency, same way as for this issue

This is a great improvement! This now needs a second approval, but after that we should not forget to create the related WFCORE issue

petrberan · 2022-10-26T08:42:47Z

Thank you for looking into this @Skyllarr .

As for the formatting, this is caused by the Apache CLI since I didn't touch the Options section at all. I remember I was thinking about not using the CLI but doing some custom formatting and I can't remember why I chose this option, if for the simple "5 lines of code vs 60" or any other reason. I could try custom formatting again if you'd like to.

As for the WFCORE, I'll create a new issue that will be merged once this one is in Elytron

petrberan · 2022-10-27T13:53:23Z

@Skyllarr Did some refactoring, the code format should be slightly better. I also created an issue for the WFCORE module changes - wildfly/wildfly-core#5257 . I presume the WFCORE PR will be merged after this one, or rather when the new Elytron module will contain this PR

petrberan · 2022-11-30T16:35:14Z

Hi @Skyllarr @fjuma can I ask for review please?

petrberan · 2023-02-13T13:40:07Z

Hi @fjuma can I ask for review please?

fjuma · 2023-03-16T20:38:00Z

@Skyllarr @cam-rod If you have a chance to try this out as well that would be great. I know @Skyllarr had previously tried it out but I think that might have been before the latest push.

jessicarod7 · 2023-03-16T21:50:47Z

Tested it out, looks good!

Skyllarr · 2023-03-17T15:22:04Z

@fjuma I just tested as well and looks good!

petrberan · 2023-04-27T12:19:04Z

@fjuma Can I ask for a review please?

fjuma · 2023-05-23T19:03:47Z

@petrberan Would you be able to update this so it's against the 2.x branch instead of the 1.x branch? Thanks and apologies for the delayed review.

petrberan · 2023-10-31T14:50:55Z

Hi @fjuma , managed to finally rebase it to 2.x

fjuma

@petrberan Apologies for the delayed review! This is looking great! I've added a few very small comments to address in the new year and then this one should be good to go. Thanks very much for working on this, it looks very nice!

fjuma · 2023-12-22T20:27:33Z

tool/src/main/java/org/wildfly/security/tool/ElytronToolMessages.java

@@ -607,4 +607,9 @@ public interface ElytronToolMessages extends BasicLogger {
    @Message(id = 35, value = "Only one of '%s' and '%s' can be specified at the same time")
    IllegalArgumentException mutuallyExclusiveOptions(String first, String second);

+    @Message(id = NONE, value = "To get list of options for specific command, please specify the command by using ./elytron-tool.sh [command] --help")


s/get list of options for specific command/get a list of options for a specific command

fjuma · 2023-12-22T20:27:48Z

pom.xml

@@ -98,6 +98,7 @@
        <version.io.rest-assured>4.3.3</version.io.rest-assured>
        <version.net.sourceforge.htmlunit.htmlunit>2.40.0</version.net.sourceforge.htmlunit.htmlunit>
        <version.org.apache.santuario>2.3.0</version.org.apache.santuario>
+        <version.org.aesh>2.4</version.org.aesh>


We should update this to the latest version.

tool/src/main/java/org/wildfly/security/tool/ElytronTool.java

fjuma · 2023-12-22T20:30:11Z

tool/src/main/java/org/wildfly/security/tool/help/CommandsSection.java

@@ -0,0 +1,116 @@
+/*
+ * JBoss, Home of Professional Open Source.
+ * Copyright 2022 Red Hat, Inc., and individual contributors


Very minor, but we can update the year in the new files (sorry for the delayed review!).

fjuma · 2023-12-22T20:41:19Z

tool/src/main/java/org/wildfly/security/tool/ElytronToolMessages.java

@@ -105,25 +105,25 @@ public interface ElytronToolMessages extends BasicLogger {
            "Provider must be installed through java.security file or through service loader from properly packaged jar file on classpath.")
    String cmdLineCustomCredentialStoreProviderDesc();

-    @Message(id = NONE, value = "Create credential store (Action)")
+    @Message(id = NONE, value = "* Create credential store")


Just wondering if we should have the * at the end instead of the beginning just to be consistent with the way Action was displayed before. Maybe it stands more at the beginning though?

@Skyllarr @petrberan What do you think?

Hi there @fjuma , thank you for your review, I'll try to get those resolved asap

Just wanted to point here the UX solution behind the *. The problem with (Action) being at the end is that since all those lines are not the same length, it's very hard for us to find out which command is action and which not, as it requires to read every single line up until the very end of said line. Us humans being very lazy, we don't generally do that :) For that reason, I'd put it in the beginning of the line.

However, since (Action) is just another word, it doesn't stand out enough to be noticed at first. By replacing (Action) with * it's something extra that moves the entire line a little bit, showing exactly which command is action and which not.

In the end, it's all about what you want exactly. If you want user to know which command is action and which not, putting * in the beginning is better. If you prefer user to read the command easily and don't really care about them knowing whether it's an action or not, it could stay in the back

Hi @petrberan, thanks for the explanation! Putting the * at the beginning makes sense then.

petrberan · 2024-01-04T17:07:56Z

Hi @fjuma , all requested changes should be resolved, the new command's formatting is set to the new style as well

fjuma

Thanks very much for the updates, @petrberan! This looks great!

fjuma · 2024-01-05T15:05:55Z

@Skyllarr Since you last reviewed this one a little while ago, please take a quick look at the updates when you get a chance and then this one should be good to go. Thanks!

Skyllarr · 2024-01-26T19:38:03Z

Hi @petrberan , I just did another sanity check on windows, but the colors when I run elytron-tool.bat on windows are not rendering and instead it looks like below:

←[1;96;109mDESCRIPTION←[0m

I am not sure why it was not this way last time I tested, but I suspect I made a mistake by trying it out in the window's git bash instead of the windows command line. Can this be resolved?

If it won't be trivial to make colors work for all OSs, it is okay IMO to not use colors in this PR, the formatting is a great improvement on its own

darranl · 2024-03-14T17:36:13Z

Hi @petrberan is it possible to follow up on the last suggestions from @Skyllarr so we can see if we can get this through?

petrberan · 2024-03-14T17:52:37Z

Hi @darranl , it's on my todo list but I've been caught on another issues, still planning to look into it. The last comment was actually resolved by adding the aesh module into WFCORE (as it should be), but the colors don't work on some of the Windows consoles, where WFLY colors work properly. Since both projects use the aesh library, that should be bug and I'll look more into it

petrberan · 2024-04-08T15:39:44Z

Managed to find the culprit. Problem was that even Aesh created the title strings properly, they displayed incorrectly on old Windows terminal because they wasn't printed by Aesh, but by the system itself. Added missing TerminalConnection and now Aesh handles printing those titles itself, working properly.

Works fine on Linux and Windows VM, can you please have a look as well? @Skyllarr Please make sure that WFCORE contains the Aesh dependency in the module.xml as well

petrberan · 2024-05-06T13:53:47Z

Hi @Skyllarr , any news?

fjuma · 2024-05-07T17:46:20Z

@ivassile Do you have access to a Windows VM or machine by any chance to try this out?

ivassile · 2024-05-07T18:39:30Z

@ivassile Do you have access to a Windows VM or machine by any chance to try this out?

I don't have Windows VM, but will setup and test it tomorrow.

fjuma · 2024-05-07T18:40:44Z

@ivassile Do you have access to a Windows VM or machine by any chance to try this out?

I don't have Windows VM, but will setup and test it tomorrow.

That would be great, thanks Ilia!

ivassile · 2024-05-09T02:54:26Z

@fjuma I tested the changes on Windows 10 and the help command displays fine.

ivassile · 2024-08-20T19:19:20Z

@petrberan Can you please rebase? Thanks!

petrberan · 2024-08-29T14:51:31Z

Rebased @ivassile

ivassile · 2024-08-29T20:43:47Z

@fjuma This PR can be merged now.

darranl · 2024-09-15T11:21:51Z

I have just put the WildFly Core PR on CI to double check, if that passes I will merge both.

IMO we don't actually need to merge together, the proposed change to WildFly Core is safe to merge on it's own.

Thank you @petrberan

fjuma reviewed Jun 8, 2022

View reviewed changes

tool/src/main/java/org/wildfly/security/tool/help/CommandsSection.java Show resolved Hide resolved

Skyllarr reviewed Jun 13, 2022

View reviewed changes

petrberan force-pushed the ELY-2082 branch from 4236e4d to 09d7e0d Compare June 13, 2022 12:56

petrberan force-pushed the ELY-2082 branch from 09d7e0d to 1e67192 Compare July 19, 2022 12:46

Skyllarr reviewed Oct 25, 2022

View reviewed changes

Skyllarr approved these changes Oct 25, 2022

View reviewed changes

Skyllarr added the +1 DV label Oct 25, 2022

petrberan force-pushed the ELY-2082 branch from 1e67192 to d9778ae Compare October 27, 2022 13:37

fjuma changed the base branch from 1.x to 2.x May 23, 2023 19:03

fjuma changed the base branch from 2.x to 1.x May 23, 2023 19:03

petrberan changed the base branch from 1.x to 2.x October 31, 2023 14:32

petrberan force-pushed the ELY-2082 branch from d9778ae to 2fd6f99 Compare October 31, 2023 14:48

petrberan requested a review from fjuma November 23, 2023 12:13

fjuma requested changes Dec 22, 2023

View reviewed changes

petrberan force-pushed the ELY-2082 branch from 2fd6f99 to 6c49690 Compare January 4, 2024 17:06

fjuma approved these changes Jan 5, 2024

View reviewed changes

fjuma added the +1 FJ label Jan 5, 2024

Skyllarr removed the +1 DV label Feb 6, 2024

Skyllarr self-requested a review February 6, 2024 14:29

petrberan force-pushed the ELY-2082 branch from 6c49690 to 877f150 Compare April 8, 2024 15:36

ivassile approved these changes Aug 20, 2024

View reviewed changes

[ELY-2082] Optimise Tool Help Text

5ed42fd

petrberan force-pushed the ELY-2082 branch from 877f150 to 5ed42fd Compare August 29, 2024 14:51

darranl added the +1 IV label Sep 15, 2024

darranl merged commit 37a290d into wildfly-security:2.x Sep 15, 2024
9 checks passed

[ELY-2082] Optimise Tool Help Text #1713

[ELY-2082] Optimise Tool Help Text #1713

Conversation

petrberan commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Skyllarr commented Jun 13, 2022 • edited Loading

petrberan commented Jun 13, 2022

Skyllarr commented Jun 27, 2022

Skyllarr commented Jun 27, 2022

petrberan commented Jun 28, 2022

Skyllarr commented Jun 28, 2022

petrberan commented Jul 19, 2022

Choose a reason for hiding this comment

Skyllarr commented Oct 25, 2022 • edited Loading

Skyllarr commented Oct 25, 2022

petrberan commented Oct 26, 2022

petrberan commented Oct 27, 2022

petrberan commented Nov 30, 2022

petrberan commented Feb 13, 2023

fjuma commented Mar 16, 2023

jessicarod7 commented Mar 16, 2023

Skyllarr commented Mar 17, 2023

petrberan commented Apr 27, 2023

fjuma commented May 23, 2023

petrberan commented Oct 31, 2023

fjuma left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

petrberan commented Jan 4, 2024

fjuma left a comment

Choose a reason for hiding this comment

fjuma commented Jan 5, 2024

Skyllarr commented Jan 26, 2024 • edited Loading

darranl commented Mar 14, 2024

petrberan commented Mar 14, 2024

petrberan commented Apr 8, 2024

petrberan commented May 6, 2024

fjuma commented May 7, 2024

ivassile commented May 7, 2024

fjuma commented May 7, 2024

ivassile commented May 9, 2024

ivassile commented Aug 20, 2024

petrberan commented Aug 29, 2024

ivassile commented Aug 29, 2024

darranl commented Sep 15, 2024

Skyllarr commented Jun 13, 2022 •

edited

Loading

Skyllarr commented Oct 25, 2022 •

edited

Loading

Skyllarr commented Jan 26, 2024 •

edited

Loading