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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add output to tedge cert create and tedge cert remove commands #1654

Merged
merged 1 commit into from
Dec 20, 2022

Conversation

Bravo555
Copy link
Contributor

Proposed changes

Added output to tedge cert create and tedge cert remove commands:

  • tedge cert create
    • Certificate was successfully created. if certificate was successfully created
  • tedge cert remove
    • Certificate was successfully removed. if certificate was removed
    • There is no certificate to remove. if there is no certificate

The documentation was also updated to include the added output.

Types of changes

  • Bugfix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Improvement (general improvements like code refactoring that doesn't explicitly fix a bug or add any new functionality)
  • Documentation Update (if none of the other choices apply)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Paste Link to the issue


Checklist

  • I have read the CONTRIBUTING doc
  • I have signed the CLA (in all commits with git commit -s)
  • I ran cargo fmt as mentioned in CODING_GUIDELINES
  • I used cargo clippy as mentioned in CODING_GUIDELINES
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

Further comments

The change was made because I think it would be good for all the CLI commands that could possibly be invoked by the human, to print human-readable output describing what happened, even if just one sentence, it's better than nothing.

E.g. tedge cert remove returned Ok for both cert removed and no cert cases, but it had no output, so the user wouldn't know what actually happened. Alternatively no cert case could return an error, but I'd argue it makes more sense as a success case because the end result is the same as with cert removed case: there is no cert and a new one can be created.

As for the implementation, i separated the logic and printing between 2 impls, but e.g. cli/certificate/show.rs just does the printing in the 2nd impl, and if it's fine, then I could also move the print there and avoid some boilerplate.

Lastly, sorry if I'm bikeshedding too hard 馃槄

@reubenmiller
Copy link
Contributor

Thanks for the PR, yes this is great. It was actually on the roadmap.

Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Only some minor things. This small improvement has a great user impact!

crates/core/tedge/src/cli/certificate/create.rs Outdated Show resolved Hide resolved
crates/core/tedge/src/cli/certificate/remove.rs Outdated Show resolved Hide resolved
@didier-wenzek
Copy link
Contributor

I personally prefer terse tools. mkdir and rm tell nothing if things went okay. Here tedge cert show can be used to check the outcome of tedge cert create/remove. Also, removing something that doesn't exist is not an error.

That said the design of the user interface is not my domain of responsibility and I will let @reubenmiller decide. The key point is to have a consistent CLI!

@reubenmiller
Copy link
Contributor

I personally prefer terse tools. mkdir and rm tell nothing if things went okay. Here tedge cert show can be used to check the outcome of tedge cert create/remove. Also, removing something that doesn't exist is not an error.

That said the design of the user interface is not my domain of responsibility and I will let @reubenmiller decide. The key point is to have a consistent CLI!

Yes I would prefer having giving the user feedback as it helps gain user trust by showing them that something was done.

But yes I agree that running tedge cert remove when there is no certificate should still result in a 0 exit code. But I still like the user message (on stderr).

Added output to `tedge cert create` and `tedge cert remove` commands:

- `tedge cert create`
  - `Certificate was successfully created.` if certificate was
    successfully created
- `tedge cert remove`
  - `Certificate was successfully removed.` if certificate was removed
  - `There is no certificate to remove.` if there is no certificate

The documentation was also updated to include the added output.

The change was made because I think it would be good for all the CLI
commands that could possibly be invoked by the human, to print
human-readable output describing what happened, even if just one
sentence, it's better than nothing.

E.g. `tedge cert remove` returned `Ok` for both `cert removed` and `no
cert` cases, but it had no output, so the user wouldn't know what
actually happened. Alternatively `no cert` case could return an error,
but I'd argue it makes more sense as a success case because the end
result is the same as with `cert removed` case: there is no cert and a
new one can be created.

As for the implementation, i separated the logic and printing between 2
impls, but e.g. `cli/certificate/show.rs` just does the printing in the
2nd impl, and if it's fine, then I could also move the print there and
avoid some boilerplate.

Lastly, sorry if I'm bikeshedding too hard 馃槄

Signed-off-by: Marcel Guzik <marcel.guzik@inetum.com>
Copy link
Contributor

@reubenmiller reubenmiller left a comment

Choose a reason for hiding this comment

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

Approved

@reubenmiller reubenmiller added improvement User value theme:cli Theme: cli related topics labels Dec 16, 2022
@didier-wenzek didier-wenzek merged commit 5073a11 into thin-edge:main Dec 20, 2022
@Bravo555 Bravo555 deleted the cli-command-output branch December 20, 2022 12:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
improvement User value theme:cli Theme: cli related topics
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants