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.

Sign up for GitHub

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

Jump to bottom

[NO QA] fix: action failure on invalid String in BASH #4049

Merged
merged 1 commit into from
Jul 19, 2021
Merged

[NO QA] fix: action failure on invalid String in BASH #4049

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions .github/workflows/deployBlocker.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ jobs:
run: |
echo "DEPLOY_BLOCKER_URL=${{ github.event.pull_request.html_url }}" >> $GITHUB_ENV
echo "DEPLOY_BLOCKER_NUMBER=${{ github.event.pull_request.number }}" >> $GITHUB_ENV
echo "DEPLOY_BLOCKER_TITLE=${{ github.event.pull_request.title }}" >> $GITHUB_ENV
echo "DEPLOY_BLOCKER_TITLE=$(sed -e "s/'/'\\\\''/g; s/\`/\\\\\`/g; 1s/^/'/; \$s/\$/'/" <<< ${{ github.event.pull_request.title }})" >> $GITHUB_ENV
roryabraham marked this conversation as resolved.
Show resolved Hide resolved

- name: Update StagingDeployCash with new deploy blocker
uses: Expensify/Expensify.cash/.github/actions/createOrUpdateStagingDeploy@main
Expand All @@ -54,7 +54,7 @@ jobs:
channel: '#deployer',
attachments: [{
color: "#DB4545",
text: '💥 New E.cash Deploy Blocker: <${{ env.DEPLOY_BLOCKER_URL }}|${{ env.DEPLOY_BLOCKER_TITLE }}>',
text: '💥 New E.cash Deploy Blocker: <${{ env.DEPLOY_BLOCKER_URL }}|'+ `${{ env.DEPLOY_BLOCKER_TITLE }}`.replace(/(^'|'$)/gi, '').replace(/'\''/gi,'\'') + '>',
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmmm I don't think this is likely to work? This isn't a JS string, it's using the expression syntax for GH actions.

Copy link
Member Author

Choose a reason for hiding this comment

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

Look at here https://action-slack.netlify.app/with#custom_payload example. We can use js.
I checked the action code its is using eval();

Copy link
Contributor

Choose a reason for hiding this comment

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

We can use js

Whoa, that's a nice feature of action-slack.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah totally agree, despite the use of eval.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also show me what local testing you did for this?

Copy link
Member Author

Choose a reason for hiding this comment

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

This one is untested apart from the js code which I tested so that it gives us the original string.
I am not too sure about the + usages here. It could be the problem. But I feel it will work based on these:

  1. I tested Yaml syntax which is valid.
  2. Github will pass the string as it is after replacing the placeholders.
  3. Now eval should parse the value. So I tested if the string is valid js syntax. Which it is after Gh action placeholders replacements.

Copy link
Contributor

Choose a reason for hiding this comment

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

Can you show me the tests you did for the JS replacement portion? Even just using a Node.js REPL and some sample strings to demonstrate how the replacement works.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Used this for YAML validation.
  2. for the eval part you can use this https://jsbin.com/roxenoyibu/edit?js,console to test out strings.
  3. You can use [NO QA] fix: action failure on invalid String in BASH #4049 (comment) to get strings.

}]
}
env:
Expand Down