-
Notifications
You must be signed in to change notification settings - Fork 29
Fix bug in skipTests #5863
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
Fix bug in skipTests #5863
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5863 +/- ##
==============================================
+ Coverage 61.78% 74.52% +12.74%
- Complexity 4277 5274 +997
==============================================
Files 369 369
Lines 19056 19056
Branches 2025 2025
==============================================
+ Hits 11774 14202 +2428
+ Misses 6300 3893 -2407
+ Partials 982 961 -21
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
.circleci/config.yml
Outdated
@@ -116,7 +116,7 @@ jobs: | |||
- run: | |||
name: evaluate [skipTests] condition | |||
command: | | |||
echo "export COMMIT_MESSAGE=\"$(git log --format=oneline -n 1 $CIRCLE_SHA1)\"" >> $BASH_ENV | |||
echo "export COMMIT_MESSAGE=\"$(git log --format=oneline -n 1 $CIRCLE_SHA1 | sed 's/\"/\\\"/g')\"" >> $BASH_ENV |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this fix will break if the commit message contains a $
, because we'll end up sourcing a line that looks something like:
export COMMIT_MESSAGE="this is a $test"
which will try to substitute the value of the test
variable. Of course, a commit message like that isn't likely, until we change the code to be vulnerable, then it'll happen the next day, lol.
Could we directly set COMMIT_MESSAGE
, without appending it to the BASH_ENV file? Something like
export COMMIT_MESSAGE="$(git log --format=oneline -n 1 $CIRCLE_SHA1)"
|
Description
[skipTests]
has a bug regarding double quotes in commit messages, discovered in this run: https://app.circleci.com/pipelines/github/dockstore/dockstore/11103/workflows/32fc7c3e-7ef6-4349-86a1-57993fec2401/jobs/42460. It looks like the command fails to process the " characters literally, causing syntax errors when exportingCOMMIT_MESSAGE
.Adding a command that substitutes instances of " with \" seems to resolve the issue, by escaping all double quotes (see https://app.circleci.com/pipelines/github/dockstore/dockstore/11107/workflows/76244033-1d18-4bb7-b748-6e76cdfb6fff/jobs/42481). On this branch, I also tried commit messages containing different special characters, to check that they can be properly exported as
COMMIT_MESSAGE
.Review Instructions
From config.yml and the test commits, verify that commit messages containing double quotes (and other common, non-alphanumeric characters) are exported correctly (i.e.
COMMIT_MESSAGE
should be outputted properly).Are there any other special characters that might be found in commit messages, which haven't been attempted and might cause similar issues to the double quote?
Issue
SEAB-5015
(https://oicr.slack.com/archives/C05EZH3RVNY/p1712251398680789)
Security and Privacy
If there are any concerns that require extra attention from the security team, highlight them here and check the box when complete.
e.g. Does this change...
Please make sure that you've checked the following before submitting your pull request. Thanks!
mvn clean install
@RolesAllowed
annotation