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

Used set to make shell scripts more strict #3278

Merged
merged 3 commits into from
May 16, 2022

Conversation

owaiskazi19
Copy link
Member

@owaiskazi19 owaiskazi19 commented May 10, 2022

Signed-off-by: Owais Kazi [email protected]

Description

Used set -e -o pipefail in shell scripts to generalize and ensure that errors are not silently hidden.

More on pipefail:
This setting prevents errors in a pipeline from being masked. If any command in a pipeline fails, that return code will be used as the return code of the whole pipeline. By default, the pipeline's return code is that of the last command even if it succeeds.

Issues Resolved

#3106

Check List

  • New functionality includes testing.
    • All tests pass
  • New functionality has been documented.
    • New functionality has javadoc added
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@owaiskazi19 owaiskazi19 requested review from a team and reta as code owners May 10, 2022 20:00
@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success e377fd627404f0913342a70bcc3ffe2960dd5381
Log 5213

Reports 5213

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 10fad09
Log 5214

Reports 5214

@andrross
Copy link
Member

I agree that -e and -o pipefail are best practices, but I'm a little concerned that we might break things by making this change across all scripts. Some of these scripts could have been written assuming the non-strict behavior. Do you think we have adequate test coverage and/or are confident that nothing here will break?

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented May 10, 2022

I agree that -e and -o pipefail are best practices, but I'm a little concerned that we might break things by making this change across all scripts. Some of these scripts could have been written assuming the non-strict behavior. Do you think we have adequate test coverage and/or are confident that nothing here will break?

That's a great question. I'm not sure if there's a way to test these scripts out. I thought gradle check would do the work for us. Any input you have on the same?
From what I have seen, few scripts are part of Dockerfile. Probably we can test the Dockerfile out by running it as a container.

@owaiskazi19
Copy link
Member Author

I tried to run these scripts just to see if they will execute and changed -o pipefail to -e.

@opensearch-ci-bot
Copy link
Collaborator

❌   Gradle Check failure 35839a79ece3130453b2559c8ef8c129d707cfb3
Log 5221

Reports 5221

@owaiskazi19
Copy link
Member Author

#2064

@owaiskazi19
Copy link
Member Author

start gradle check

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 35839a79ece3130453b2559c8ef8c129d707cfb3
Log 5225

Reports 5225

@andrross
Copy link
Member

I tried to run these scripts just to see if they will execute and changed -o pipefail to -e.

Why did you make this change? Did -o pipefail cause problems?

Signed-off-by: Owais Kazi <[email protected]>
@owaiskazi19
Copy link
Member Author

Why did you make this change? Did -o pipefail cause problems?

Yes, when executed on my local returned

> ./dev-tools/signoff-check.sh 
./dev-tools/signoff-check.sh: 3: set: Illegal option -o pipefail

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success c285109
Log 5234

Reports 5234

@rursprung
Copy link
Contributor

what about -u? (see the original issue as well)

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented May 13, 2022

what about -u? (see the original issue as well)

Hey @rursprung! With -eu flag on few scripts I'm getting the below error. That's why thought of standardizing with -e as of now because -u flag will require more handling. WDYT?

> Task :rest-api-spec:yamlRestTest FAILED
Exec output and error:
| Output for ./bin/opensearch-keystore:./bin/opensearch-env: line 48: OPENSEARCH_JAVA_HOME: unbound variable

@rursprung
Copy link
Contributor

rursprung commented May 13, 2022

then i'd suggest to fix them: if you explicitly want to use variable substitution, see e.g. here: https://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_06_02

so instead of writing $FOO (which would error with set -u if FOO is undefined) you write ${FOO:-} (to default to an empty string or ${FOO:-bar} (to default to the string "bar"). as listed on that page (and many others on the web) there are various options available through this, incl. more advanced ones (e.g. using other variables in the fallback text). that way you can properly control the behaviour.

the specific error you mentioned seems to come from this check for whether the variable is set:

if [ ! -z "$OPENSEARCH_JAVA_HOME" ]; then

as outlined in this answer the check should probably be written slightly differently: https://stackoverflow.com/questions/3601515/how-to-check-if-a-variable-is-set-in-bash/13864829#13864829
note that ! -z should anyway just be -n.

so i guess something like this should do the trick:

if [ -n "${OPENSEARCH_JAVA_HOME:-}" ]; then

an experienced shell script developer might have better ideas though :)

but i'd strongly suggest to not ignore failures due to unset variables, they can be really tricky to track down if they're not intentional (as with this check which is just a slightly misguided null-check).

EDIT: greatly simplified the check suggestion - don't know why i suggested something more complicated before

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented May 14, 2022

@rursprung I have added a generalized way for scripts to set using -e -o pipefail. Handled set: Illegal option -o pipefail for ./dev-tools/signoff-check.sh by changing.

@opensearch-ci-bot
Copy link
Collaborator

✅   Gradle Check success 2ce05f9
Log 5331

Reports 5331

@rursprung
Copy link
Contributor

@rursprung I have added a generalized way for scripts to set using -e -o pipefail. Handled set: Illegal option -o pipefail for ./dev-tools/signoff-check.sh by changing.

good news, thanks! i missed that some still had #!/bin/sh - obviously #!/usr/bin/env bash is the correct way of doing things.

what about -u (my previous comment which i've now just updated with a better suggestion)?

@owaiskazi19
Copy link
Member Author

owaiskazi19 commented May 14, 2022

what about -u (my previous comment which i've now just updated with a better suggestion)?

If we use set -eu -o pipefail for the script returns the below. That's mainly because of. How do you like the current way of having set -e -o pipefail?

> Task :test:fixtures:krb5kdc-fixture:postProcessFixture FAILED

FAILURE: Build failed with an exception.

* Where:
Build file '/home/ubuntu/OpenSearch/test/fixtures/krb5kdc-fixture/build.gradle' line: 52

* What went wrong:
Execution failed for task ':test:fixtures:krb5kdc-fixture:postProcessFixture'.
> Cannot get property 'test.fixtures.peppa.udp.88' on extra properties extension as it does not exist

@dblock
Copy link
Member

dblock commented May 16, 2022

@rursprung looking for your A-OK to merge this, thank you for doing a CR

@rursprung
Copy link
Contributor

if possible, i'd like to see -u in there as well - i'm not a big fan of silently accepting undefined variables in the code. yes, it works in a lot of scripting languages, but it just adds a (hard to track) source of errors. i'm not sure about the last failure reported by @owaiskazi19 - i don't see anything shell-script related in the error message but also have no clue what this test covers (and don't have time in the next few days to check it out myself, sorry).

if there are corner cases (special scripts) where -u isn't possible then i think it should still be added to all others and an explicit comment should be added to the corner cases explaining why/how they rely on undefined variables.

but if this doesn't work for you / you don't want that i think the PR is already a good step in the right direction!

one more thing to consider: maybe it'd make sense to add a small check (GitHub action?) to ensure that the set command with the specified minimum of restrictions is present in all .sh files? and maybe this should be documented somewhere? otherwise this here is a one-off effort and in new scripts this will be forgotten again.

@owaiskazi19
Copy link
Member Author

@rursprung @dblock created #3343 for the above. We can go ahead and get this merge if everything looks fine.

@dblock dblock merged commit e73a410 into opensearch-project:main May 16, 2022
@dblock dblock added the backport 2.x Backport to 2.x branch label May 16, 2022
opensearch-trigger-bot bot pushed a commit that referenced this pull request May 16, 2022
* Use set to make shell scripts more strict

Signed-off-by: Owais Kazi <[email protected]>

* Change -o pipefail to -e

Signed-off-by: Owais Kazi <[email protected]>

* Set scripts to standard rule

Signed-off-by: Owais Kazi <[email protected]>
(cherry picked from commit e73a410)
owaiskazi19 added a commit that referenced this pull request May 16, 2022
* Use set to make shell scripts more strict

Signed-off-by: Owais Kazi <[email protected]>

* Change -o pipefail to -e

Signed-off-by: Owais Kazi <[email protected]>

* Set scripts to standard rule

Signed-off-by: Owais Kazi <[email protected]>
(cherry picked from commit e73a410)

Co-authored-by: Owais Kazi <[email protected]>
@owaiskazi19 owaiskazi19 mentioned this pull request Jun 2, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x Backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants