Add CLI version command prefix #11474
Conversation
There was a problem hiding this comment.
Welcome to LocalStack! Thanks for raising your first Pull Request and landing in your contributions. Our team will reach out with any reviews or feedbacks that we have shortly. We recommend joining our Slack Community and share your PR on the #community channel to share your contributions with us. Please make sure you are following our contributing guidelines and our Code of Conduct.
|
All contributors have signed the CLA ✍️ ✅ |
This comment was marked as duplicate.
This comment was marked as duplicate.
|
I have read the CLA Document and I hereby sign the CLA |
There was a problem hiding this comment.
The change itself is looking good, clean
8000
and simple.
However, I think this chance could cause quite a few issues considering how this command is being used by our users. A plain "version only" output can be very handy when you use the output of the command in scripts.
When just looking at the usage of the command in a quick GitHub code search, we can see that there are quite a lot of usages, some of which might break with this change:
https://github.com/search?q=%22localstack+--version%22&type=code
Is there a chance we could make this distinction between the core emulator runtime and the CLI versions a bit more clear without modifying the output here directly?
And concerning the failing pipeline: You can fix the formatting by running make format, and can make sure that this is done automatically on every commit by enabling the pre-commit hook with make init-precommit.
deb25d5 to
103a884
Compare
Great points!
I'd like to challenge the belief that the version only output is proving more value, but leaving this decision to you and the rest of the engineering group! I'd be also happy to close this PR and revisit in the future! 🏓
I don't have a better idea for this—let's close the PR unless someone else wantw to pick it up to try this. 📕
Force pushed a commit to fix this! Thanks for the hint, @alexrashed! 💛 |
|
lets take it to the next level, by providing even moar information and help the user to achieve what they might have come for. 🚀 This all is based on my assumption that me as a user typing I understand that we don't know the version of localstack right away, but still we can tell the user about it and how to find out. something like this |
|
briefly looked over the search results @alexrashed posted. I'd guess people intent to get the emulator version most of the time instead of cli version. |
I am not sure what you want to say by that. Do you mean "people are having a UX issue because they get something else than they expect"? |
|
Thanks for your feedback, @SimonWallner and @alexrashed! 💛 I opened this as a minimal viable change (MVC) out of confusion around the output of the version command, but it seems this could benefit from an alignment discussion, probably in a different medium (Slack, meeting, etc). 🛹 Taking a bias for action and closing this PR to be respectful of everyone's time. 🏓 |
103a884 to
c160d91
Compare
|
Reopened this, following up from relevant discussion (internal). The intention was to include a link to https://localstack.cloud/releases (see relevant discussion (internal)), but maybe this creates more confusion than help, especially when linking to our blog (see relevant discussion (internal)). 💭 So, I was thinking could we merge this as is as a minimal viable change (MVC) iteration? We can always come back to improve this more, or even consider making this output more dynamic, see relevant discussion (internal). What do you think, @SimonWallner and @alexrashed? |
|
Currently, only minor and patch changes are allowed on master. Your PR labels (semver: major) indicate that it cannot be merged into the master at this time. |
|
Sorry for the notification noise, everyone! 💜 |
ed4d4e3 to
2c3492f
Compare
There was a problem hiding this comment.
Awesome! Nice and clean! Thanks a lot for the patience and the good discussion on how to get this in the release! 🥇
The changes here are already mentioned in the pinned issue (#11803), preparing our users for the upcoming change! 💯
Congrats to your first contribution to LocalStack! 🥳
|
FYI: The failing pipeline is unrelated and is addressed in #11808. I'll directly move forward and merge this PR into our release branch. |
|
Thanks @alexrashed! For visibility, I've updated the target branch, rebased, and made the following changes:
I've also updated the BEFORE/AFTER screenshots on the PR description to reflect the latest changes. Thanks, everyone! 🎊 |
Motivation
Besides LocalStack releases, CLI is also released with semantic versioning.
This can be confusing to users, see relevant discussions[1][2][3][...] (internal).
Cc @SimonWallner @HarshCasper @dfangl @inothnagel @whummer
The CLI version command outputs a semantic number, but does not specify if this is referring to LocalStack or CLI.
Changes
This will add LocalStack CLI as a prefix before the version number and style the message to be more subtle and different from the rest of the output, so that the version command is more explicit and can be used later on in every command invocation for reference.