-
Notifications
You must be signed in to change notification settings - Fork 3.1k
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
Add production/staging deploy comment #2444
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.
function getDeployTableMessage(platformResult) { | ||
const emoji = platformResult === 'success' ? '✅' : '❌'; | ||
return platformResult + emoji; | ||
} | ||
|
||
const androidResult = getDeployTableMessage(core.getInput('ANDROID', {required: true})); | ||
const desktopResult = getDeployTableMessage(core.getInput('DESKTOP', {required: true})); | ||
const iOSResult = getDeployTableMessage(core.getInput('IOS', {required: true})); | ||
const webResult = getDeployTableMessage(core.getInput('WEB', {required: true})); | ||
|
||
const workflowURL = `${process.env.GITHUB_SERVER_URL}/${process.env.GITHUB_REPOSITORY} | ||
/actions/runs/${process.env.GITHUB_RUN_ID}`; | ||
|
||
let message = `🚀 [Deployed](${workflowURL}) 🚀 to | ||
${isProd ? 'production' : 'staging'} on ${date.toDateString()} at ${date.toTimeString()}`; | ||
|
||
message += `\n\n platform | result \n ---|--- \n android|${androidResult} \n desktop|${desktopResult}`; | ||
message += `\n iOS|${iOSResult} \n web|${webResult}`; | ||
|
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.
NAB: Curious if it's possible/worth DRY-ing up these blocks of code?
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.
Which lines specifically did you have in mind? The four instances of getDeployTableMessage
? I could loop through them and store them in a map? Did you have any other ideas?
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 there's something I'm missing but looks to me that these lines are the same in the markPullRequestsAsDeployed.js
, just wondering if we could have this logic in some sort of helper function?
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 there's something I'm missing but looks to me that these lines are the same in the markPullRequestsAsDeployed.js
, just wondering if we could have this logic in some sort of helper function?
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.
Ahh - Sorry this is confusing, but index.js
is just a compiled JS version for GitHub actions with all the libraries, etc needed to run. So yes it will be identical to the source code (which is not index.js
). So you can just ignore all the code in index.js as it was compiled via npm run gh-actions-build
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 pretty 🔥, just a few NAB comments.
|
||
const octokit = github.getOctokit(token); | ||
const githubUtils = new GithubUtils(octokit); | ||
|
||
function getDeployTableMessage(platformResult) { |
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.
NAB could do this instead:
function getDeployTableMessage(platformResult) {
switch (platformResult) {
case 'success':
return `${platformResult} ✅`;
case 'cancelled':
return `${platformResult} 🔪`;
case 'skipped':
return `${platformResult} 🚫`;
case 'failure':
default:
return `${platformResult} ❌`;
}
}
Also, you're missing a method doc comment.
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.
Yeah we could, I was thinking anything but a success is a ✅ - But more emojis the better, so let's do it
.github/actions/markPullRequestsAsDeployed/markPullRequestsAsDeployed.js
Outdated
Show resolved
Hide resolved
.github/actions/markPullRequestsAsDeployed/markPullRequestsAsDeployed.js
Outdated
Show resolved
Hide resolved
Co-authored-by: Rory Abraham <47436092+roryabraham@users.noreply.github.com>
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.
LGTM 👍
In order to test this before QA starts I am going to merge this now. E2E is not done, but this does not change any of that code. |
✋ This PR was not deployed to staging yet because QA is ongoing. It will be automatically deployed to staging after the next production release. |
🚀 [Deployed](https://github.com/Expensify/Expensify.cash
|
Details
Adds a production or staging deploy comment when the deploy is over with a table of what platforms deployed successfully and which did not.
Fixed Issues
Fixes https://github.com/Expensify/Expensify/issues/157261
Tests (No QA)