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

[ADR] JAMES-3400 James CLI based on webadmin endpoints #251

Closed

Conversation

quantranhong1999
Copy link
Contributor

ADR for James CLI based on webadmin endpoints.

@quantranhong1999 quantranhong1999 changed the title JAMES-3400 ADR for James CLI based on webadmin endpoints [ADR] JAMES-3400 James CLI based on webadmin endpoints Oct 5, 2020
@quantranhong1999 quantranhong1999 marked this pull request as draft October 5, 2020 08:40
@quantranhong1999 quantranhong1999 marked this pull request as ready for review October 5, 2020 08:40
Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Can you just keep the changes concerning the ADR here, using git rebase ?

Also stash them all in a single commit!

Thanks in advance ;-)

@chibenwa
Copy link
Contributor

chibenwa commented Oct 5, 2020

Please can you add the ticket number as a prefix?

JAMES-3400 the rest of the message

You can do that with rebase + reword

Copy link
Contributor

@Arsnael Arsnael left a comment

Choose a reason for hiding this comment

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

Overall nice job :)

src/adr/0042-james-cli-based-on-webadmin.md Show resolved Hide resolved
src/adr/0042-james-cli-based-on-webadmin.md Outdated Show resolved Hide resolved
src/adr/0042-james-cli-based-on-webadmin.md Outdated Show resolved Hide resolved
Copy link
Member

@mbaechler mbaechler left a comment

Choose a reason for hiding this comment

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

This is a good thing to finally switch admin to http.

However we should drop legacy things like jar+lib packaging, making people running java -jar ..., etc

Also, I would add:

  • write a man page (asciidoc can generate it for you)
  • provide the right tool to enable shell completion

But I would already be happy if none of my comments were adopted.


## Decision

We decided to write a new CLI client, running on top of the JVM, communicating with James via the webadmin protocol, using http.
Copy link
Member

Choose a reason for hiding this comment

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

Do you intend to make it buildable with GraalVM native image in order to make the adoption easier and have decent start performance?
It's a matter of avoiding not-supported stuff but for a CLI it should be doable.

Copy link
Contributor

Choose a reason for hiding this comment

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

Agree with distributing this CLI as a GraalVM native image 👍

Looks easily achievable https://www.graalvm.org/reference-manual/native-image/NativeImageMavenPlugin/


We decided to write a new CLI client, running on top of the JVM, communicating with James via the webadmin protocol, using http.

* What libraries will we use?
Copy link
Member

Choose a reason for hiding this comment

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

I think it's not wise to list libraries here as it's an implementation detail and it would require a new ADT to modify them is people find that the choices validated here don't work as expected.

Copy link
Contributor

Choose a reason for hiding this comment

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

Later ADRs can superseed this and overide the original choice?

Copy link
Member

Choose a reason for hiding this comment

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

It's just too many details, as far as I know we never wrote an ADR about our libraries choices.


* How will we limit breaking changes this new CLI will cause?

* Work on a wrapper to adapt the old CLI API.
Copy link
Member

Choose a reason for hiding this comment

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

good idea 👍

Copy link
Contributor

Choose a reason for hiding this comment

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

credits to @rouazana :-)


* Where will we locate this cli code?

* server/protocols/webadmin-cli
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't make sense to me: it's not part of the server, it's a standalone app.

What about app/cli?

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO that's a promotion that could come later

Copy link
Member

Choose a reason for hiding this comment

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

could you argue about putting it inside server/protocols? I can't find any rational for that.

Copy link
Contributor

Choose a reason for hiding this comment

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

It is related to webadmin, having it close to webadmin feels natural to me.

Putting something that is not (yet) the official cli in an official cli place might be confusing. I would defer that choice.

* We decided to adopt a more modern, modular CLI syntax:

```
$ java -jar james_cli.jar [OPTION] ENTITY ACTION {ARGUMENT}
Copy link
Member

Choose a reason for hiding this comment

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

oh no, please drop this java -jar ... thing

Copy link
Contributor

Choose a reason for hiding this comment

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

What do you propose instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, graalVM allows to avoid that

Copy link
Contributor

Choose a reason for hiding this comment

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

Quan can you replace java -jar james_cli.jar by james-cli ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done it

@chibenwa
Copy link
Contributor

chibenwa commented Oct 7, 2020

write a man page (asciidoc can generate it for you)

Picocli generates beautiful documentation for your application (HTML, PDF and Unix man pages)

Will be easy, let's add the mention!

* We decided to adopt a more modern, modular CLI syntax:

```
$ java -jar james_cli.jar [OPTION] ENTITY ACTION {ARGUMENT}
Copy link
Contributor

Choose a reason for hiding this comment

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

Quan can you replace java -jar james_cli.jar by james-cli ?

src/adr/0042-james-cli-based-on-webadmin.md Outdated Show resolved Hide resolved
src/adr/0042-james-cli-based-on-webadmin.md Outdated Show resolved Hide resolved
Copy link
Contributor

@chibenwa chibenwa left a comment

Choose a reason for hiding this comment

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

Almost there!

Let's update the README as well so that I can merge it all at once ;-)

server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
server/protocols/webadmin-cli/README.md Outdated Show resolved Hide resolved
@chibenwa
Copy link
Contributor

chibenwa commented Oct 8, 2020

Merged

Quan, can you close this?

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