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

Merged
merged 1 commit into from
Nov 22, 2024

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.

@AntonRoskvist
Copy link
Contributor Author

@clebertsuconic @gemmellr
Hope these changes are to your liking.

For a future change I've also looked a bit into adding command history
Not really sure how to verify that properly though as I have no access to a Windows PC, but I think it should be fairly trivial to implement.

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 maven-assembly-plugin added to the artemis-cli/pom.xml but I suspect that might not be the best way to do it as part of building the project in its entirety. I also don't really know if just a jar file is "fine" to distribute or if we want additional packaging to make the tool as intuitive and user friendly as possible.

Do you have any thoughts on this?

@gemmellr
Copy link
Member

gemmellr commented Nov 22, 2024

@clebertsuconic @gemmellr Hope these changes are to your liking.

For a future change I've also looked a bit into adding command history Not really sure how to verify that properly though as I have no access to a Windows PC, but I think it should be fairly trivial to implement.

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 maven-assembly-plugin added to the artemis-cli/pom.xml but I suspect that might not be the best way to do it as part of building the project in its entirety. I also don't really know if just a jar file is "fine" to distribute or if we want additional packaging to make the tool as intuitive and user friendly as possible.

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.

@clebertsuconic
Copy link
Contributor

very nice addition @AntonRoskvist
Regarding any historic usage.. I wouldn't use the journal for that. I think it would be too complex for something simple.. I would rather use use a clear text storage as it's no big deal if you lose it... (IMO at least).. without adding circular dependencies... etc.

But we can have a discussion on the dev list.

@clebertsuconic clebertsuconic merged commit 78c816b into apache:main Nov 22, 2024
8 checks passed
@clebertsuconic
Copy link
Contributor

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)

@AntonRoskvist
Copy link
Contributor Author

@gemmellr

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?

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 artemis-cli module, then upload that to the Downloads page together with the broker distribution.

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.

@gemmellr
Copy link
Member

That is essentially what I thought you meant, so I wouldnt change anything of my overall earlier reply.

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