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] Fix Comment on issues GH action. #3937

Merged
merged 5 commits into from
Jul 12, 2021

Conversation

parasharrajat
Copy link
Member

Details

It's a follow up PR for #3771 (comment)

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 July 8, 2021 23:53
@MelvinBot MelvinBot requested review from NikkiWines and removed request for a team July 8, 2021 23:54
@parasharrajat
Copy link
Member Author

parasharrajat commented Jul 8, 2021

@roryabraham this is an attempt to fix the break.

  1. Currently I have added logging to see the real issue. And Noticed that PRList is String[] so we need to convert to a number[] for matching. I follow strictly matching (===).
  2. I found one interesting fact that isStagingDeployLocked returns currently opened StagingDeployIssue issue with LockDeploycash label. But we don't want to get that. Instead, we should pick the StagingDeployIssue which was active when PR was merged. Any idea, how can I get that?

@roryabraham
Copy link
Contributor

Instead, we should pick the StagingDeployIssue which was active when PR was merged. Any idea, how can I get that?

Unfortunately, I don't think there's any really easy way to do it. Maybe something like this pseudocode:

const mergedAt = PR.merged_at;
const allStagingDeployCashIssues = getAllStagingDeployCashIssues().sortBy('newestToOldest');
let stagingDeployCash = null;
for (let issue of allStagingDeployCashIssues) {
    if (issue.created < mergedAt) {
        // This is the first StagingDeployCash created before this PR was merged.
        // Therefore it was the one that was active when the PR was merged.
        stagingDeployCash = issue;
        break;
    }
}

@parasharrajat
Copy link
Member Author

🚀 Deployed to staging in version: test🚀

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

@parasharrajat
Copy link
Member Author

Update: I have fixed the core issue which broke the GH action in the first place. Now I am doing changes related to #3937 (comment) comment.

To test the Changes I will use my old PR #3771 and one more with CP label. Please ignore the Comments like #3937 (comment).

Thank you.

@roryabraham
Copy link
Contributor

roryabraham commented Jul 12, 2021

@parasharrajat Did you forget to push your changes here? Let us know when this is ready for review? I think it's still using the non-existent export called GithubUtils.GITHUB_REPOSITORY

@parasharrajat
Copy link
Member Author

Yeah, I am doing the last few changes to get the correct StagingDeployIssue and then I will push those changes all along.

@parasharrajat
Copy link
Member Author

@roryabraham Updated and tested on #3581. Seems to work fine.
I have tried to minimize the Github API calls to a minimum best of my knowledge.

sort: 'created',
direction: 'desc',
labels: GithubUtils.STAGING_DEPLOY_CASH_LABEL,
per_page: 30,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think 30 per page is the default, right?

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

parasharrajat and others added 2 commits July 12, 2021 12:52
…eployed.js

Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@parasharrajat
Copy link
Member Author

Updated

Copy link
Contributor

@NikkiWines NikkiWines left a comment

Choose a reason for hiding this comment

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

👍

@NikkiWines NikkiWines merged commit 4c4ff1f into Expensify:main Jul 12, 2021
@OSBotify
Copy link
Contributor

✋ 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
Copy link
Contributor

🚀 Deployed to staging in version: 1.0.77-6🚀

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

@OSBotify
Copy link
Contributor

🚀 Deployed to production in version: 1.0.79-4🚀

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

@parasharrajat parasharrajat deleted the gh-actions branch November 20, 2023 13:07
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