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

ARTEMIS-5168 Improve remoting to brokers from Artemis shell #5359

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AntonRoskvist
Copy link
Contributor

This PR adds back connect + disconnect to shell
Changes prompt to reflect if connected to a broker (and which one)
It also captures "Ctrl + D" to return to shell if issued when connected to a broker (instead of exiting). If not connected it exits as usual.

@gemmellr

This comment was marked as outdated.

@clebertsuconic
Copy link
Contributor

I really liked this...

can I ask a small change.. if you can't do it.. I will do it later...

if the user is null.. just don't set the user

If I have a broker without an user (anonymous user).. it's currently showing null@tcp://localhost:61616 >

I think it should use just tcp://localhost:61616 >

although the check that you have on the Exception would need to be different.. perhaps just use a boolean to connected instead.

t's currently showing this, when null:

null@tcp://localhost:61616 >

@clebertsuconic
Copy link
Contributor

I had commented on the wrong PR.. I just posted here correctly now..

@AntonRoskvist let me know if you can change the null, and the exception handling to use some boolean state rather than the string of the connection / prompt.

@AntonRoskvist
Copy link
Contributor Author

AntonRoskvist commented Nov 21, 2024

Thanks @gemmellr

@clebertsuconic I actually had it slightly differently at one point where I set the user as "anonymous@tcp://..." referencing "--allow-anonymous" from the create command. I opted against it because just prior the shell will log this:
getActionContext().out.println("CLI connected to broker " + brokerURL + ", user:" + user);
(where the user also displays as null in this case)

I can go with either but I feel like that logging should be consistent and also log "user: anonymous" or no user at all.

I am fine making the change either way but it might be a day or two before I can get to it (hectic schedule)

@clebertsuconic
Copy link
Contributor

I'm fine with anonymous@...

I just didn't like the null

to me either anonymous@URL. or not mention the URL is fine... it's just cosmetics ... I like either way.

@clebertsuconic
Copy link
Contributor

I get your point with logging... but I think since this prompt will be there all the time.. it makes it kind of ugly :)

We can fix the logging as well.. I will wait your PR to see where you get first.

@gemmellr
Copy link
Member

I'm fine with anonymous@...

I just didn't like the null

to me either anonymous@URL. or not mention the URL is fine... it's just cosmetics ... I like either way.

I'd be inclined not to assume and display 'anonymous' as a user. As you can set the URL, you might for example be using client certificates, and then saying anonymous would just be incorrect.

@clebertsuconic
Copy link
Contributor

clebertsuconic commented Nov 21, 2024

@gemmellr : hmmm... good point... so lets just not display the user at all.... just the URL if the username is not set.

I wouldn't fix the logging.. as for the logging null is the correct output.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants