fix: safely replace description placeholder in janitor run #116
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Before this commit a (single-)quote or the pipe symbol in the description would make the janitor fail to replace the description placeholder as those made the sed statement malformed. A first fix is to store the value of the description in a variable and pass that to the sed statement. This eliminates the problems of quotes and variable expansion due to $ signs. The variable had to be passed as a real variable instead of using githubs variable replacement as with that the shell would still see the quotes and parse them. The second thing to fix is to escape any occurences of the pipe symbol in the description as that is what the sed command uses as its delimiter. The escaping has to be done by adding a literal \ to the description so during the sed command the | will stay in place. While doing this it became clear that also the \ has to be escaped as it would otherwise vanish after the sed run. Thus this is done prior to escaping the pipe symbol in order to not also mess with the introduced \ in that command. To verify it is working the test step gained a new variable that is injected if the it is no new repo. With this at least it is checked that the description can contain the special characters. One thing to note though is that setting the test variable and extracting the actual description need to look alike so we can be sure the actual retrieval will not fail.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #116 +/- ##
=======================================
Coverage 20.50% 20.50%
=======================================
Files 5 5
Lines 239 239
Branches 112 109 -3
=======================================
Hits 49 49
- Misses 179 181 +2
+ Partials 11 9 -2
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this commit a (single-)quote or the pipe symbol in the description would make the janitor fail to replace the description placeholder as those made the sed statement malformed.
A first fix is to store the value of the description in a variable and pass that to the sed statement. This eliminates the problems of quotes and variable expansion due to $ signs. The variable had to be passed as a real variable instead of using githubs variable replacement as with that the shell would still see the quotes and parse them.
The second thing to fix is to escape any occurences of the pipe symbol in the description as that is what the sed command uses as its delimiter. The escaping has to be done by adding a literal \ to the description so during the sed command the | will stay in place. While doing this it became clear that also the \ has to be escaped as it would otherwise vanish after the sed run. Thus this is done prior to escaping the pipe symbol in order to not also mess with the introduced \ in that command.
To verify it is working the test step gained a new variable that is injected if the it is no new repo. With this at least it is checked that the description can contain the special characters. One thing to note though is that setting the test variable and extracting the actual description need to look alike so we can be sure the actual retrieval will not fail.
fixes #114