Change Shellcheck severity level to warning and make fixes #2611
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #2611 +/- ##
========================================
Coverage 75.56% 75.56%
========================================
Files 16 16
Lines 2132 2132
========================================
Hits 1611 1611
Misses 521 521
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
| @@ -73,7 +72,7 @@ function print_block_device_mapping { | |||
| function check_instance_store { | |||
| if ls /dev/nvme* >& /dev/null; then | |||
| IS_NVME=1 | |||
| MAPPINGS=$(realpath --relative-to=/dev/ -P /dev/disk/by-id/nvme*Instance_Storage* | grep -v "*Instance_Storage*" | uniq) | |||
| MAPPINGS=$(realpath --relative-to=/dev/ -P /dev/disk/by-id/nvme*Instance_Storage* | grep -v "Instance_Storage" | uniq) | |||
There was a problem hiding this comment.
For this shellcheck warning SC2063, I deleted the *'s as advised, but I am unsure what behavior we want:
Before, the grep -v "*Instance_Storage*" wouldn't filter out any results, because the * at the beginning wouldn't match anything, so the statement would've been the same as MAPPINGS=$(realpath --relative-to=/dev/ -P /dev/disk/by-id/nvme*Instance_Storage* | uniq). Is that what we want instead?
With the deletion of the *'s, MAPPINGS would be empty, because grep will filter out all results since all the results contain Instance_Storage.
There was a problem hiding this comment.
Let's do this:
MAPPINGS=$(realpath --relative-to=/dev/ -P /dev/disk/by-id/nvme*Instance_Storage* | grep -v "\*Instance_Storage\*" | uniq)
Explanation: The command does not filter out any results on instance types without instance store. If the command is run on instance types with instance store, there will be results.
The -v in grep is revert filter. This is to filter out /dev/disk/by-id/nvme*Instance_Storage* from realpath when running on instance types without instance store.
My proposal uses \ to escape * to tell shellcheck the *s are not Regex keywords
I usually sub-command one by one to understand a command so big :)
There was a problem hiding this comment.
Ohh got it, thanks for the explanation. I have updated the line
util/cinc-install.sh
Outdated
| @@ -75,6 +75,7 @@ checksum_mismatch() { | |||
| unable_to_retrieve_package() { | |||
| echo "Unable to retrieve a valid package!" | |||
| report_bug | |||
| # shellcheck disable=SC2154 | |||
| echo "Metadata URL: $metadata_url" | |||
There was a problem hiding this comment.
I am unsure where metadata_url comes from
There was a problem hiding this comment.
As you can see in the first comment of this bash script, this file is a copy of the https://omnitruck.cinc.sh/install.sh file, with some modification (you can see the modification in the comments on top of the file).
During the modification we removed the part to download the installer from cinc website and we're downloading it from S3 instead. You can remove all the lines were we're printing the metadata_url, it's useless for us.
There was a problem hiding this comment.
Makes sense, I deleted the lines with metadata_url. Thanks!
Signed-off-by: Judy Ng <[email protected]>
Signed-off-by: Judy Ng <[email protected]>
Signed-off-by: Judy Ng <[email protected]>
Signed-off-by: Judy Ng <[email protected]>
Signed-off-by: Judy Ng <[email protected]>
Description of changes
Tests
References
Checklist
developadd the branch name as prefix in the PR title (e.g.[release-3.6]).Please review the guidelines for contributing and Pull Request Instructions.
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.