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] Combine all platform deploys into one big file #2376

Merged
merged 2 commits into from
Apr 15, 2021

Conversation

AndrewGable
Copy link
Contributor

Details

We initially created separate platform deploy files to make things easier to read, however, it's making it harder to debug failures and also making it harder to post when certain jobs are done. This is a first step in improving both of those things.

Fixed Issues

Related (does not 100% fix) https://github.com/Expensify/Expensify/issues/157261

Tests

  1. Merge this when the checklist is not locked
  2. I will verify that iOS, Web, Desktop, and Android finish deploying
  3. I will also verify that the failure messaging is working

QA Steps

No QA

@AndrewGable AndrewGable requested a review from a team as a code owner April 13, 2021 20:52
@AndrewGable AndrewGable self-assigned this Apr 13, 2021
@MelvinBot MelvinBot requested review from bondydaa and removed request for a team April 13, 2021 20:53
Copy link
Contributor

@roryabraham roryabraham left a comment

Choose a reason for hiding this comment

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

Overall looks good, just had a few comments. I think it's likely this breaks something, but we'll find out when we merge it.

.github/workflows/platformDeploy.yml Show resolved Hide resolved
.github/workflows/platformDeploy.yml Show resolved Hide resolved
.github/workflows/platformDeploy.yml Outdated Show resolved Hide resolved
ios/Podfile.lock Show resolved Hide resolved
.github/workflows/platformDeploy.yml Show resolved Hide resolved
attachments: [{
color: "#DB4545",
pretext: `<!here>`,
text: `💥 ${process.env.AS_REPO} failed on ${process.env.AS_WORKFLOW} workflow 💥`,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe the message should list which jobs failed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Any ideas on how? The text is kinda hard to change as you can only use their environment variables I think https://action-slack.netlify.app/fields

Copy link
Contributor

Choose a reason for hiding this comment

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

Meh, probably not worth the ugly bash it would take to make this work.

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 something like this might work tho:

run: |
  FAILED_PLATFORMS=()
  ${{ needs.android.status === 'failed' }} && FAILED_PLATFORMS+="android"
  ${{ needs.ios.status === 'failed' }} && FAILED_PLATFORMS+="ios"
  ${{ needs.web.status === 'failed' }} && FAILED_PLATFORMS+="web"
  ${{ needs.desktop.status === 'failed' }} && FAILED_PLATFORMS+="desktop"
  echo "FAILED_PLATFORMS=$($FAILED_PLATFORMS | sed  's/ /, /g')" >> $GITHUB_ENV

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And then can you access process.env.FAILED_PLATFORMS?

SLACK_WEBHOOK_URL: ${{ secrets.SLACK_WEBHOOK }}

postGithubComment:
name: Post a GitHub comment when platforms are done building and deploying
Copy link
Contributor

Choose a reason for hiding this comment

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

Note to @bondydaa that right now this job just logs a message, doesn't actually post anything in GitHub. I think @AndrewGable is planning on doing that in a follow-up.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Correct, this is just a first step PR.

Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
@@ -402,6 +402,8 @@ PODS:
- React-Core
- RNCAsyncStorage (1.12.1):
- React-Core
- RNCClipboard (1.5.1):
Copy link
Contributor

Choose a reason for hiding this comment

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

someone forget to add this lock file when updating the podfile? said differently why no podfile changes?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, correct, that is my assumption

Copy link
Contributor

Choose a reason for hiding this comment

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

👍 do we have botify posts in dot cash? like we've solved this already in mobile and web-e right by having botify post when you're missing one or the other so maybe we just need to update the webhook to include dotcash too?

@roryabraham roryabraham merged commit 02dff94 into master Apr 15, 2021
@roryabraham roryabraham deleted the andrew-combine-deploys branch April 15, 2021 16:15
@roryabraham roryabraham changed the title Combine all platform deploys into one big file [No QA] Combine all platform deploys into one big file Apr 15, 2021
@OSBotify
Copy link
Contributor

🚀 [Deployed](https://github.com/Expensify/Expensify.cash
/actions/runs/765130740) 🚀 to
staging on Mon Apr 19 2021 at 22:41:56 GMT+0000 (Coordinated Universal Time)

platform result
🤖 android 🤖 success ✅
🖥 desktop 🖥 success ✅
🍎 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