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

Modify JMH Benchmark Workflow For Shellcheck #813

Merged
merged 10 commits into from
Aug 21, 2023
Merged

Conversation

armughan11
Copy link
Collaborator

@armughan11 armughan11 commented Aug 21, 2023

Fixed Shellcheck errors for JMH workflow files.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

Thanks for this! Mostly looks great, but I have a couple comments

@@ -1,9 +1,9 @@
#!/bin/bash

cd $BRANCH_NAME/
cd "$BRANCH_NAME/" || exit
Copy link
Collaborator

@msridhar msridhar Aug 21, 2023

Choose a reason for hiding this comment

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

For all these cases of || exit I'm concerned that if the command ever fails (which I don't expect) it will be very difficult to diagnose. Does shellcheck still complain about the missing || exit if you do #!/bin/bash -eux at the beginning of the scripts?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Using -eux is probably a better way to do it for our case. Updated accordingly

.buildscript/check_git_clean.sh Outdated Show resolved Hide resolved
@msridhar
Copy link
Collaborator

BTW, a general tip with git is to make changes in your fork on a branch other than master. A separate named branch makes it easier to keep up to date when changes are pushed to the upstream main branch and makes it easier for others to add commits to your branch as needed.

Copy link
Collaborator

@msridhar msridhar left a comment

Choose a reason for hiding this comment

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

LGTM thanks!

@msridhar msridhar enabled auto-merge (squash) August 21, 2023 17:12
@msridhar msridhar merged commit 7cf3e3b into uber:master Aug 21, 2023
7 checks passed
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.

2 participants