-
Notifications
You must be signed in to change notification settings - Fork 3k
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
Conversation
There was a problem hiding this 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.
attachments: [{ | ||
color: "#DB4545", | ||
pretext: `<!here>`, | ||
text: `💥 ${process.env.AS_REPO} failed on ${process.env.AS_WORKFLOW} workflow 💥`, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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): |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
🚀 [Deployed](https://github.com/Expensify/Expensify.cash
|
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
QA Steps
No QA