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

--help fail on config json error #2301

Open
wants to merge 23 commits into
base: master
Choose a base branch
from
Open

Conversation

ATorrise
Copy link
Contributor

@ATorrise ATorrise commented Oct 9, 2024

What It Does
Ensures users can still access the help documentation and version details, even when config files contain errors.

How to Test

  1. Create a zowe.config.json file with invalid JSON or syntax errors.
  2. Run a Zowe CLI command with the --help flag, ie: zowe zosmf --help
  3. Confirm that the help text is displayed with a warning about the invalid JSON.
  4. Run the same command without the --help flag to ensure it still fails as expected with a JSON error.

Review Checklist
I certify that I have:

Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 91.15%. Comparing base (440f8e3) to head (71b032a).

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #2301   +/-   ##
=======================================
  Coverage   91.15%   91.15%           
=======================================
  Files         636      636           
  Lines       18029    18042   +13     
  Branches     3767     3776    +9     
=======================================
+ Hits        16434    16447   +13     
  Misses       1594     1594           
  Partials        1        1           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Signed-off-by: ATorrise <amber.torrise@broadcom.com>
Signed-off-by: ATorrise <amber.torrise@broadcom.com>
@ATorrise ATorrise changed the title all needed changes to meet AC besides tests --help fail on config json error Oct 22, 2024
@ATorrise ATorrise marked this pull request as ready for review October 22, 2024 20:54
Signed-off-by: Amber Torrise <amber.torrise@broadcom.com>
Signed-off-by: Amber Torrise <amber.torrise@broadcom.com>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

A few tests failing, but changes LGTM! 😋

Curious if we agreed on having the error at the bottom of the output of towards the top.
Just seeing it in practice, someone could miss the error if they are viewing the help text 😋

packages/imperative/CHANGELOG.md Outdated Show resolved Hide resolved
packages/imperative/src/config/src/Config.ts Outdated Show resolved Hide resolved
packages/imperative/src/config/src/doc/IConfigLayer.ts Outdated Show resolved Hide resolved
zowe-cli Outdated Show resolved Hide resolved
packages/imperative/src/config/src/api/ConfigLayers.ts Outdated Show resolved Hide resolved
packages/imperative/src/config/src/api/ConfigLayers.ts Outdated Show resolved Hide resolved
packages/imperative/src/config/src/Config.ts Outdated Show resolved Hide resolved
packages/imperative/src/config/src/Config.ts Outdated Show resolved Hide resolved
ATorrise and others added 2 commits October 30, 2024 08:33
Co-authored-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Amber Torrise <112635587+ATorrise@users.noreply.github.com>
Signed-off-by: Amber Torrise <amber.torrise@broadcom.com>
@ATorrise
Copy link
Contributor Author

ATorrise commented Nov 5, 2024

was testing a revert to see if error in test persisted

adding a note so that theres less confusion when reviewing... i went back in my commit history locally by doing a git reset –soft [commit hash] to check if my failing tests were still failing. rather than deleting this local branch and re downloading from remote, i instead pushed all those changes back up, making several commits and merges look like 1 commit in an attempt to restore the progress made.

ATorrise and others added 4 commits November 5, 2024 10:24
Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
…hat was missed in imperative cleanup

Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
…msg-help-version

Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
zFernand0 and others added 2 commits November 5, 2024 10:39
Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
@zFernand0
Copy link
Member

Added the no-chagelog label since the zosfiles sdk change was just to address a lint warning

  • The changelog was not changed in this pull request for packages/zosfiles/CHANGELOG.md.

Signed-off-by: Amber <amber.torrise@broadcom.com>
Signed-off-by: Fernando Rijo Cedeno <37381190+zFernand0@users.noreply.github.com>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

LGTM! 😋
Sorry for giving you so much trouble on the reviews 😋

Signed-off-by: Amber <amber.torrise@broadcom.com>
@zFernand0 zFernand0 dismissed t1m0thyj’s stale review November 5, 2024 20:20

I believe these changes were addressed 😋

Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Looks pretty good, thanks @ATorrise - made a few small requests

Signed-off-by: Amber <amber.torrise@broadcom.com>
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

Thanks for addressing feedback, will approve once the unit test is fixed 😋

Signed-off-by: Amber <amber.torrise@broadcom.com>
Copy link
Contributor

@anaxceron anaxceron left a comment

Choose a reason for hiding this comment

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

Left some notes for minor edits to the changelog entry

packages/imperative/CHANGELOG.md Outdated Show resolved Hide resolved
Signed-off-by: Amber <amber.torrise@broadcom.com>
Copy link
Member

@zFernand0 zFernand0 left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback and for being patient 🙏

LGTM! 😋

Signed-off-by: Timothy Johnson <timothy.johnson@broadcom.com>
Copy link
Member

@t1m0thyj t1m0thyj left a comment

Choose a reason for hiding this comment

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

When testing the PR I was still seeing a fatal error, which I fixed in 71b032a 🙂

Please add an integration test that runs zowe --help and zowe --version with a bad config to verify the error is non-fatal.

@t1m0thyj t1m0thyj dismissed their stale review November 6, 2024 21:49

original requests were addressed

Copy link

sonarcloud bot commented Nov 6, 2024

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Review/QA
Development

Successfully merging this pull request may close these issues.

Zowe commands with --help fail if config file cannot be parsed
4 participants