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

Add CI validation of shell scripts using shellcheck #4826

Conversation

akagami-harsh
Copy link
Member

@akagami-harsh akagami-harsh commented Oct 8, 2023

Which problem is this PR solving?

Resolves #4822

Description of the changes

  • fixed all 83 warnings produced by shellcheck
  • added a workflow for shellcheck

How was this change tested?

  • CI

Checklist

@akagami-harsh akagami-harsh requested a review from a team as a code owner October 8, 2023 13:26
@akagami-harsh akagami-harsh changed the title Feature 4822 ci validation of shell script Feature 4822 CI validation of shell scripts using shellcheck Oct 8, 2023
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

I reviewed the first few files, the same mistake is repeated over and over. When a variable is meant to represent multiple arguments like x="a b c", using it in quotes "${x}" means all those values will be given to the command as a single argument, the same as passing a\ b\ c. This breaks the semantic of most of the scripts.

scripts/build-crossdock.sh Outdated Show resolved Hide resolved
scripts/build-upload-a-docker-image.sh Outdated Show resolved Hide resolved
scripts/cassandra-integration-test.sh Show resolved Hide resolved
scripts/cassandra-integration-test.sh Show resolved Hide resolved
@akagami-harsh
Copy link
Member Author

I reviewed the first few files, the same mistake is repeated over and over. When a variable is meant to represent multiple arguments like x="a b c", using it in quotes "${x}" means all those values will be given to the command as a single argument, the same as passing a\ b\ c. This breaks the semantic of most of the scripts.

@yurishkuro Thank you for taking the time to review my code and providing valuable feedback. I was so fixated on resolving those shellcheck warnings, that I forgot it was breaking the sole purpose of the code. I revisited all of it again and made some changes , can you review it again

Signed-off-by: Harshvir Potpose <[email protected]>
Signed-off-by: Harshvir Potpose <[email protected]>
@akagami-harsh
Copy link
Member Author

can you please add the Hacktoberfest label

@akagami-harsh
Copy link
Member Author

@yurishkuro , i have made some changes, can you please review it.

@codecov
Copy link

codecov bot commented Oct 15, 2023

Codecov Report

All modified lines are covered by tests ✅

📢 Thoughts on this report? Let us know!.

scripts/compute-tags.sh Outdated Show resolved Hide resolved
scripts/compute-tags.sh Outdated Show resolved Hide resolved
akagami-harsh and others added 4 commits October 17, 2023 15:26
Signed-off-by: Harshvir Potpose <[email protected]>
Signed-off-by: Harshvir Potpose <[email protected]>
Signed-off-by: Harshvir Potpose <[email protected]>
@yurishkuro yurishkuro added the changelog:ci Change related to continuous integration / testing label Oct 18, 2023
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
Signed-off-by: Yuri Shkuro <[email protected]>
@yurishkuro yurishkuro changed the title Feature 4822 CI validation of shell scripts using shellcheck Add CI validation of shell scripts using shellcheck Oct 19, 2023
Copy link
Member

@yurishkuro yurishkuro left a comment

Choose a reason for hiding this comment

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

looking good! 🎉

@yurishkuro yurishkuro merged commit 1b6fecc into jaegertracing:main Oct 19, 2023
33 checks passed
@yurishkuro
Copy link
Member

Thanks!

@akagami-harsh akagami-harsh deleted the feature-4822-ci-validation-of-shell-script branch October 22, 2023 14:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
changelog:ci Change related to continuous integration / testing
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature]: Implement CI validation of shell scripts using shellcheck
2 participants