-
Notifications
You must be signed in to change notification settings - Fork 924
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
Conversation
This comment was marked as outdated.
This comment was marked as outdated.
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 > |
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. |
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: 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) |
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 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. |
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. |
@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. |
da14f28
to
8080685
Compare
@clebertsuconic @gemmellr For a future change I've also looked a bit into adding command history Finally I'd also be interested in building this as a standalone tool and distribute it on the website, alongside the broker distribution and the native binaries. I've build this for my own purposes with Do you have any thoughts on this? |
I wouldn't be in favour of it personally. The CLI already depends on a large proportion of the broker distribution so there would be a significant overlap. Given the way they exist in the overall build and are typically used, the benefits of a standalone CLI distribution really dont seem that significant to me to warrant actually maintaining such a thing. Some uses actively dont seem to make sense given their relation to the broker / instance configuration. Which makes me think..have I actually entirely misunderstood what you are asking about? The native bits for the journal only have their own downloads because they are actually released independently, infrequently, and so requires its own source assembly (the source is always the 'actual release'). The convenience binary is only there because its being built so the output can be consumed in the actual broker convenience binaries later during Artemis builds. Its off on its own sub-download page without any wider site component detail since its not really expected anyone actually needs to download it. Thoughts of a 'new' component distribution would also really be for discussion on the mailing list, rather than just a simple improvement PR. |
very nice addition @AntonRoskvist But we can have a discussion on the dev list. |
I thought you were proposing using the journal... just read what you wrote properly.. (so igrnoe my comment from 1 minute ago...) The CLI is very intrinsict dependent with the broker as Robbie said.. I'm not sure how we would make it standalone... it would be a lot of effort for little gain IMO. (we can have a separate discussion) |
You might have misunderstood, but personally I don't know what's practical or even possible here so it might just be a bad idea to begin with... What I am proposing is not a separate distribution entirely but rather to build an additional executable jar file in the current Then it will get maintained and used within the project just as it is now, while giving users easy access to a standalone tool for managing brokers. |
That is essentially what I thought you meant, so I wouldnt change anything of my overall earlier reply. |
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.