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

[No QA] Switch warnCPLabel trigger from pull_request to pull_request_target #4370

Merged
merged 2 commits into from
Aug 2, 2021

Conversation

roryabraham
Copy link
Contributor

Details

The warnCPLabel.yml workflow has been failing on any PR from a fork. This PR should fix that.

More discussion here.

Fixed Issues

$ n/a

Tests

  1. Merge this PR.
  2. Create a PR and give it the CP Staging label. OSBotify should comment on the PR explaining the implications of the CP Staging Label.
  3. Create a PR from a fork of Expensify/App and give it the CP Staging label. OSBotify should comment on the PR explaining the implications of the CP Staging Label.

Tested On

n/a – Live testing in GH only.

@roryabraham roryabraham self-assigned this Aug 2, 2021
@roryabraham roryabraham requested a review from a team as a code owner August 2, 2021 18:00
@MelvinBot MelvinBot requested review from luacmartins and removed request for a team August 2, 2021 18:01
@AndrewGable
Copy link
Contributor

Does it still need OS_BOTIFY_TOKEN to make a comment or would a github token do it?

@roryabraham
Copy link
Contributor Author

I think just GITHUB_TOKEN would do it

@roryabraham
Copy link
Contributor Author

Changing it would also have the side-effect of the comment not being left by OSBotify, but that's nbd if you think there's a reason to use only the GITHUB_TOKEN instead.

@AndrewGable
Copy link
Contributor

Which is "more secure" or "more secret"?

@roryabraham
Copy link
Contributor Author

I think OS_BOTIFY_TOKEN has some additional permissions so is therefore "more secret". I can update this PR to change the token used.

Copy link
Contributor

@luacmartins luacmartins left a comment

Choose a reason for hiding this comment

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

LGTM!

@AndrewGable AndrewGable merged commit e511b1f into main Aug 2, 2021
@AndrewGable AndrewGable deleted the Rory-WarnCPLabelAsGithub branch August 2, 2021 19:49
@roryabraham
Copy link
Contributor Author

Okay, beginning testing now

@AndrewGable
Copy link
Contributor

One reminder for the test steps, I think your main you branch or fork off of has to be up to date to have this updated workflow run

@OSBotify
Copy link
Contributor

OSBotify commented Aug 2, 2021

✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release.

@AndrewGable
Copy link
Contributor

So if we see this fail on contributor fork's, I think they will have to merge main in

@roryabraham roryabraham mentioned this pull request Aug 2, 2021
@roryabraham
Copy link
Contributor Author

Step 2 (regression test) passed: #4380

@roryabraham roryabraham mentioned this pull request Aug 2, 2021
@roryabraham
Copy link
Contributor Author

Nice, step 3 passed: #4381

@OSBotify
Copy link
Contributor

OSBotify commented Aug 6, 2021

🚀 Deployed to staging in version: 1.0.82-8🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

@OSBotify
Copy link
Contributor

🚀 Deployed to production by @francoisl in version: 1.0.83-1 🚀

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 failure ❌
🍎 iOS 🍎 success ✅
🕸 web 🕸 success ✅

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.

4 participants