Skip to content
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

[WFCORE-3187] add-user.sh tool prints unexpected characters if arrow keys are pushed during interactive mode #2715

Closed
wants to merge 10 commits into from

Conversation

MMarus
Copy link
Contributor

@MMarus MMarus commented Aug 16, 2017

JBEAP: https://issues.jboss.org/browse/JBEAP-6640
WFCORE: https://issues.jboss.org/browse/WFCORE-3187

Please DON'T merge before #2533 is merged, since this pull request requires changes introduced in #2533 .

@wildfly-ci
Copy link

Can one of the admins verify this patch?

@MMarus
Copy link
Contributor Author

MMarus commented Aug 16, 2017

The first 9 commits are from #2533 . The 78d45d0 addresses the WFCORE-3187 and JBEAP-6640 issues which are subjects of this PR.

@MMarus MMarus force-pushed the add-user-fixes branch 2 times, most recently from 78d45d0 to 4d35dc8 Compare August 18, 2017 06:16
try {
createTerminalConnection(readLineHandler);
} catch (IOException e) {
e.printStackTrace();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this should log a proper error - you can find an example on the line 65 before change

assertEquals(Thread.State.TERMINATED, aeshJavaConsoleTestResource.getState());
}

public class AeshJavaConsoleTestResource extends Thread {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

public inner classes are generally considered as not a good practice, is there any reason this class needs to be public? Also please make it static

@darranl
Copy link
Contributor

darranl commented Nov 16, 2017

I think this one will be easier to review after a rebase.

@bstansberry bstansberry added rebase-this This PR must be rebased before it is merged security and removed 4.x labels Nov 16, 2017
@MMarus
Copy link
Contributor Author

MMarus commented Jan 25, 2018

I moved this PR to a new rebased PR #3064 for easier review.

@MMarus MMarus closed this Jan 25, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
rebase-this This PR must be rebased before it is merged security
Projects
None yet
6 participants