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] Added CP checking for deployed PR notfication #3771

Merged
merged 5 commits into from
Jul 1, 2021
Merged

[NO QA] Added CP checking for deployed PR notfication #3771

merged 5 commits into from
Jul 1, 2021

Conversation

parasharrajat
Copy link
Member

@parasharrajat parasharrajat commented Jun 27, 2021 •
edited
Loading

Details

Fixed Issues

$ Fixes #3383

Tests

QA Steps

Tested On

  • Web
  • Mobile Web
  • Desktop
  • iOS
  • Android

Screenshots

Web

Mobile Web

Desktop

iOS

Android

@parasharrajat parasharrajat requested a review from a team as a code owner June 27, 2021 19:52
@MelvinBot MelvinBot requested review from sketchydroide and removed request for a team June 27, 2021 19:52
@parasharrajat parasharrajat changed the title [NO QA] [WIP] Added CP checking for deployed PR notfication [NO QA] Added CP checking for deployed PR notfication Jun 30, 2021
@parasharrajat
Copy link
Member Author

@roryabraham Ready for review.

@parasharrajat
Copy link
Member Author

Updated.

roryabraham
roryabraham reviewed Jul 1, 2021
View reviewed changes
.github/actions/markPullRequestsAsDeployed/markPullRequestsAsDeployed.js
*/
prList.reduce((promise, pr) => promise.then(() => commentPR(pr)), Promise.resolve());
const run = function () {
return Promise.all([
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this be Promise.allSettled?

Copy link
Member Author

Choose a reason for hiding this comment

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

We need both promises to be successful otherwise it will not work. Thus .all

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, gotcha. I got those two switched in my head 🙃

@roryabraham
Copy link
Contributor

As always, GHA things are live-tested, so we'll need to monitor and verify that this actually works.

@roryabraham roryabraham merged commit 8e0c6f5 into Expensify:main Jul 1, 2021
@OSBotify
Copy link
Contributor

OSBotify commented Jul 1, 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.

@OSBotify OSBotify mentioned this pull request Jul 7, 2021
Closed
57 tasks
@roryabraham
Copy link
Contributor

Sorry for the week-delayed notice, but it does seem that this broke our deploy comment. Every platformDeploy workflow run since this was merged has had a silent error in the Comment on issues step.

@roryabraham roryabraham mentioned this pull request Jul 8, 2021
Closed
@parasharrajat
Copy link
Member Author

Ok. I will take a look.

@parasharrajat parasharrajat mentioned this pull request Jul 8, 2021
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@roryabraham roryabraham roryabraham approved these changes

@sketchydroide sketchydroide Awaiting requested review from sketchydroide

Assignees

@parasharrajat parasharrajat

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Hold for Payment on July 22] OSBotify falsely marks pull requests as cherry-picks instead of standard deploys
3 participants
@parasharrajat @roryabraham @OSBotify