Skip to content
This repository has been archived by the owner on Oct 10, 2023. It is now read-only.

disable printing usage info for management-cluster and cluster commands if they fail even for correct command #2253

Merged
merged 1 commit into from
May 19, 2022

Conversation

chandrareddyp
Copy link
Contributor

@chandrareddyp chandrareddyp commented Apr 28, 2022

What this PR does / why we need it

tanzu commands - tanzu cluster and tanzu management-cluster commands and their sub-commands, print usage information if the command fails irrespective of the reason, even though the input command is correct. The command might fail because of some internal or external errors, but commands still print command usage information that is misleading the end-user, it is not a great user experience.

As part of this fix, the usage information will be disabled (silentUsage enabled), in case the command fails, it does not print usage information just prints error details. This is applicable for tanzu cluster and tanzu management-cluster commands and their sub-commands.

Which issue(s) this PR fixes

Fixes #1907

Describe testing done for PR

Unit test cases are written to test the silentUsage for management-cluster and cluster commands and their sub-commands.
Manual testing has been done for management-cluster and cluster commands and their sub-commands, to make sure usage information does not print if the command fails because of some internal or external errors.

Release note

`tanzu cluster` and `tanzu management-cluster` commands and their sub-commands, do not print usage information in case the command fails for any reason, it just prints error information.  To get usage information for  `tanzu cluster` or `tanzu management-cluster` commands and their sub-commands, need to call the command or sub-commands with the flag `--help` or `-h`.

PR Checklist

  • Squash the commits into one or a small number of logical commits
  • Use good commit messages
  • Ensure PR contains terms all contributors can understand and links all contributors can access

Additional information

Special notes for your reviewer

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220428040200/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220428040352/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220428040549/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220428041607/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220428041737/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

}

func initClusterSilentModeUsecases() {
createClusterNegTestSilentMode = clitest.NewTest("create cluster", "cluster create", FuncToExecAndValidateStdError)
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: are we running each of these commands without args and relying on some being failure cases and some not to exercise the usage silencing behavior?
It may be a good idea group the test cases, and to provide some explanation what we are trying to testing here

Copy link
Contributor Author

@chandrareddyp chandrareddyp May 4, 2022

Choose a reason for hiding this comment

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

All cases are failure cases only, yes it's to exercise the silentUsage. I have already grouped all silent use cases, I will update details in the comments.

@vuil
Copy link
Contributor

vuil commented Apr 29, 2022

changes look good, I just have a nit suggestion posted as a comment.
Besides that, please also:

  • provide a clearer description in the commit message on what is changed. e.g. good to summarize the output behavior under various circumstances
  • add a release note line in the PR description since this change has user-facing implications. Release automation actually picks it these relnotes lines
  • sign the commit (git commit --amend -s)
  • mentioned Fixes: #xxx in the commit message

@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/silentusage branch 2 times, most recently from 27f19e7 to 7fd9e50 Compare May 4, 2022 22:18
@github-actions
Copy link

github-actions bot commented May 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220504221933/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chandrareddyp chandrareddyp force-pushed the topic/cpamuluri/silentusage branch 3 times, most recently from 1e31092 to 31b0ab6 Compare May 4, 2022 22:24
@github-actions
Copy link

github-actions bot commented May 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220504223002/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented May 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220504223459/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented May 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220504223519/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@github-actions
Copy link

github-actions bot commented May 4, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220504223525/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chandrareddyp
Copy link
Contributor Author

@vuil I have updated the commit message (with sign and issue id) and release note in PR description. Please review it. thanks.

@github-actions
Copy link

github-actions bot commented May 5, 2022

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220505010432/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@vuil
Copy link
Contributor

vuil commented May 6, 2022

/test upgrade-vc7

will merge on this passing

@vuil vuil added area/ux UX related area/cli labels May 6, 2022
@alfredthenarwhal
Copy link
Collaborator

@vuil: /test upgrade-vc7
Commit: 24adcd5

Tests failed! Build no: 1941

@vuil
Copy link
Contributor

vuil commented May 9, 2022

/test upgrade-vc7

Copy link
Contributor

@vuil vuil left a comment

Choose a reason for hiding this comment

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

lgtm, thanks!

@vuil
Copy link
Contributor

vuil commented May 12, 2022

/test upgrade-vc7

@alfredthenarwhal
Copy link
Collaborator

@vuil: /test upgrade-vc7
Commit: 24adcd5

Tests failed! Build no: 1977

@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220517003852/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@chandrareddyp
Copy link
Contributor Author

/test upgrade-vc7

@alfredthenarwhal
Copy link
Collaborator

@chandrareddyp: /test upgrade-vc7

Tests can be triggered only by the trusted reviewers (users mentioned in CODEOWNERS file).

…entUsage:true) for management-cluster and cluster commands, their sub-commands.

This gives better user experience, before this fix usage info prints always if command fails, irrespective of the issue, which is not great end user experience.
More details:
`tanzu cluster` and `tanzu management-cluster` commands and its sub-commands, do not print usage information in case command fails for any reason, it just prints error information.
To get usage details for both `tanzu cluster` and `tanzu management-cluster` commands and their sub-commands, need to call the command or sub-commands with the flag `--help` or `-h`.
eg:
`tanzu cluster --help`
`tanzu management-cluster create --help`
@github-actions
Copy link

Cluster Generation A/B Results:
https://storage.googleapis.com/tkg-clustergen/2253/20220518041700/clustergen.diff.txt
Author/reviewers:
Please review to verify that the effects on the generated cluster configurations are exactly what the PR intended, and give a thumbs-up if so.

@vuil
Copy link
Contributor

vuil commented May 18, 2022

/test upgrade-vc7

@alfredthenarwhal
Copy link
Collaborator

@vuil: /test upgrade-vc7
Commit: 8902e74

Tests failed! Build no: 2013

@chandrareddyp
Copy link
Contributor Author

chandrareddyp commented May 19, 2022

We did rerun the BOLT test, its successful - https://gitlab.eng.vmware.com/TKG/bolt/bolt-release-yamls/-/merge_requests/2013

@vuil vuil added the ok-to-merge PRs should be labelled with this before merging label May 19, 2022
@vuil vuil merged commit 40532e4 into vmware-tanzu:main May 19, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area/cli area/ux UX related cla-not-required ok-to-merge PRs should be labelled with this before merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

management-cluster and cluster commands prints usage info every time which is misleading
4 participants