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

Better Handle JSON Configuration Exceptions #12

Merged

Conversation

mjenglish
Copy link
Contributor

@mjenglish mjenglish commented Sep 16, 2022

Previously, when running the program with an invalid JSON configuration file, the loading of the JSON failed silently and the user was left clueless.

In this pull request, I suggest catching specific exception types and handling them with the appropriate logic.

Now, given an invalid JSON input, the logger displays clear error messages to the user of what went wrong.

@jskazinski
Copy link
Member

One of the ramifications of this suggested change is that the RedfishConfig() no longer catches any exceptions thrown, so all callers of RedfishConfig() will now have to catch any exceptions and this would mean changes to 5 other callers of RedfishConfig(). So in this case, handling the exception in a single place is much cleaner that requiring all callers to handle exceptions.
Another change is the inclusion of a prompt to read input from stdin using the input() function which results in no longer being able to run automatic test cases which would pause forever or until timed out, diminishing the value of this tool for executing Redfish commands.
Having a better error message, and configuration file validation, would be an improvement.

@jskazinski
Copy link
Member

As a practice with each code change, this project is following semantic versioning, so the version.py and CHANEGLOG.md should be updated with every pull request.

@mjenglish
Copy link
Contributor Author

@jskazinski Thanks for the feedback.

I forgot about the CHANGELOG.md. I will add that.

Regarding your other comments, we will not merge this code in its current state, but I am going to keep this PR open and try to incorporate your feedback when I have time.

@mjenglish mjenglish force-pushed the handle-json-configuration-exceptions branch 3 times, most recently from 2e1cee4 to ff36d5c Compare October 6, 2022 18:35
@mjenglish
Copy link
Contributor Author

Having a better error message, and configuration file validation, would be an improvement.

@jskazinski I updated this pull request. Could you please take another look?

Copy link
Member

@jskazinski jskazinski left a comment

Choose a reason for hiding this comment

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

Changes look good to me. Thanks for the improvement. Please update the version.py file also and feel free to merge.

CHANGELOG.md Show resolved Hide resolved
@mjenglish mjenglish force-pushed the handle-json-configuration-exceptions branch from b51fdb6 to 06d99f2 Compare October 12, 2022 18:15
@mjenglish mjenglish merged commit d6b40b1 into Seagate:master Oct 17, 2022
@mjenglish mjenglish deleted the handle-json-configuration-exceptions branch October 17, 2022 17:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants