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

Deploy-blockers can't be added if they include double-quotes. #3600

Closed
roryabraham opened this issue Jun 15, 2021 · 14 comments
Closed

Deploy-blockers can't be added if they include double-quotes. #3600

roryabraham opened this issue Jun 15, 2021 · 14 comments
Assignees
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2

Comments

@roryabraham
Copy link
Contributor

roryabraham commented Jun 15, 2021

If you haven’t already, check out our contributing guidelines for onboarding and email contributors@expensify.com to request to join our Slack channel!


Action Performed:

  1. Create an issue or pull request with double-quotes in the title.
  2. Give it the DeployBlockerCash workflow.

Expected Result:

  1. The deployBlocker.yml workflow should not fail.
  2. The StagingDeployCash checklist should be updated to include the new deploy blocker.

Actual Result:

Workflow failures:

https://github.com/Expensify/Expensify.cash/runs/2833882293?check_suite_focus=true
https://github.com/Expensify/Expensify.cash/runs/2833925987?check_suite_focus=true

Workaround:

Add the deploy blocker to the StagingDeployCash manually.

Platform:

Where is this issue occurring?

Github only

Version Number: 1.0.69-0
Notes/Photos/Videos: I think we just need to escape the string variable of special characters here

view this job on upwork

@roryabraham roryabraham added Engineering Monthly KSv2 Improvement Item broken or needs improvement. labels Jun 15, 2021
@roryabraham
Copy link
Contributor Author

This will also fail if the issue title contains any single quotes

@parasharrajat
Copy link
Member

It just needs text escaping which should escape every invalid and special char in bash.

@jboniface
Copy link

@roryabraham should this be external? i'm currently going down the list of unassigned orphaned issues

@roryabraham
Copy link
Contributor Author

Yep, thanks for the follow-up @jboniface!

@roryabraham roryabraham added the External Added to denote the issue can be worked on by a contributor label Jul 12, 2021
@MelvinBot
Copy link

Triggered auto assignment to @jboniface (External), see https://stackoverflow.com/c/expensify/questions/8582 for more details.

@MelvinBot MelvinBot added Daily KSv2 and removed Monthly KSv2 labels Jul 12, 2021
@parasharrajat
Copy link
Member

Do you expect it to be purely in BASH or node script would be fine?

@roryabraham
Copy link
Contributor Author

pure bash would be cleaner if there's a reasonable way to do it. We don't want to have to create another GH Action just to escape a string. Another option would be to find a community action and use that.

@parasharrajat
Copy link
Member

parasharrajat commented Jul 12, 2021

No need for action. We can directly run the node -e there or Node script.js as that is bash. I think that Node is available in PATH.

@roryabraham
Copy link
Contributor Author

I think you need to first run the setup-node action before node is available. node -e sounds fine to me, if that works.

@MelvinBot
Copy link

Triggered auto assignment to @tgolen (Exported), see https://stackoverflow.com/c/expensify/questions/7972 for more details.

@jboniface jboniface added Weekly KSv2 and removed Daily KSv2 labels Jul 12, 2021
@parasharrajat
Copy link
Member

parasharrajat commented Jul 14, 2021

Proposal

  1. We have to escape [`, '] chars to allow any strings to be used in bash.
  2. We will update https://github.com/Expensify/Expensify.cash/blob/9a23847aa430d30c093418b9b2c0511d61b6a243/.github/workflows/deployBlocker.yml#L34
    TO

Put the whole string in single quotes

This works for all chars except the single quote itself. To escape the single quote, close the quoting before it, insert the single quote, and re-open the quoting. Then as we have to escape the ` so that they can be substituted further.

 echo "DEPLOY_BLOCKER_TITLE=$(sed -e "s/'/'\\\\''/g; s/\`/\\\\\`/g; 1s/^/'/; \$s/\$/'/" <<< ${{ github.event.pull_request.title }})" >> $GITHUB_ENV

Now we unescape it to get the normal string at https://github.com/Expensify/Expensify.cash/blob/9a23847aa430d30c093418b9b2c0511d61b6a243/.github/workflows/deployBlocker.yml#L57

TO

text: '💥 New E.cash Deploy Blocker: <${{ env.DEPLOY_BLOCKER_URL }}|'+ `${{ env.DEPLOY_BLOCKER_TITLE }}`.replace(/(^'|'$)/gi, '').replace(/'\''/gi,'\'') + '>',

This should work based on the assumption that custom_payload value is parsable which I believe it is. https://action-slack.netlify.app/with#custom_payload

@tgolen
Copy link
Contributor

tgolen commented Jul 14, 2021

Heh, sounds good to me. It's a bit over my head, but sounds like you and Rory know what's going on here. 🟢

@roryabraham
Copy link
Contributor Author

Hmmm I'm having a bit of difficulty understanding this proposal as well. Don't we also have to escape double-quotes? We can continue discussion in the PR.

@jboniface
Copy link

paid

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Engineering External Added to denote the issue can be worked on by a contributor Improvement Item broken or needs improvement. Weekly KSv2
Projects
None yet
Development

No branches or pull requests

5 participants