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

adapt error handling #40

Merged
merged 2 commits into from
May 15, 2019
Merged

adapt error handling #40

merged 2 commits into from
May 15, 2019

Conversation

pb82
Copy link
Contributor

@pb82 pb82 commented May 15, 2019

The resources script has a few conflicts with the new error handling:

  1. It relied on unbound variables. Fixed by using default values
  2. It relies on functions with non-zero return codes which will cause the script to be terminated with the -eo pipefail option set. So for this function those options are temporarily unset.

@pb82 pb82 requested review from odra and pawelpaszki May 15, 2019 13:49
@@ -13,7 +13,7 @@ function check_resource {
# was returned but no actual results. That would be at
# least two lines: one for the header and one for each
# resource found
if [ "$result" -eq "1" ]; then
if [[ "$result" -eq "1" ]]; then
Copy link
Contributor

Choose a reason for hiding this comment

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

any reason you had to change it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My IDE suggests this, no immediate reason though

Copy link
Contributor

Choose a reason for hiding this comment

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

The previous way (one []) is posix standard which enables people with non linux machines to run those scripts locally (such as macs) - could you please revert it to the old way?

@pb82 pb82 merged commit 7fe7fdd into master May 15, 2019
@pb82 pb82 deleted the fix-resources-error-handling branch May 15, 2019 16:19
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