DOP-4599: Deploy-all slack button deploys all active versions of repos #1042
Conversation
|
Your feature branch infrastructure has been deployed! Your webhook URL is: https://9tvbube0zh.execute-api.us-east-2.amazonaws.com/prod/webhook/githubEndpoint/trigger/build For more information on how to use this endpoint, follow these instructions. |
anabellabuckvar
changed the title
.github/workflows/deploy-stg-ecs.yml
Outdated
| @@ -3,6 +3,7 @@ on: | |||
| branches: | |||
| - "main" | |||
| - "integration" | |||
| - "DOP-4599" | |||
There was a problem hiding this comment.
This will be deleted upon PR approval
src/services/slack.ts
Outdated
| values['repo_option'].push( | ||
| ...group.options.map((option) => { | ||
| if (!option.text.text.startsWith('(!inactive)')) return option; | ||
| }) | ||
| ); |
There was a problem hiding this comment.
We should use the filter method for this rather than map so that we don't end up with undefined values in the array
| @@ -66,7 +81,7 @@ async function deployRepo(deployable: Array<any>, logger: ILogger, jobRepository | |||
| try { | |||
| await jobRepository.insertBulkJobs(deployable, jobQueueUrl); | |||
| } catch (err) { | |||
| logger.error('deployRepo', err); | |||
| console.error('Deploy repo error'); | |||
There was a problem hiding this comment.
Could we still log the error? It can sometimes be such a blessing.
There was a problem hiding this comment.
Happy to, but it won't make much of a difference in this case! The only place we can view the output for the Slack Deploy Repo lambda is in its corresponding CloudWatch group. The output for console.error looks like the following, and logging the error would just change the format slightly:
There was a problem hiding this comment.
Fascinating! I wonder if that's always the case... I lean towards keeping the error logged just in case it can sometimes be helpful. But good catch!
src/services/slack.ts
Outdated
| for (const group of optionGroups) { | ||
| values['repo_option'].push( | ||
| ...group.options.filter((option) => { | ||
| !option.text.text.startsWith('(!inactive)'); |
There was a problem hiding this comment.
We should return this to ensure things are being filtered as expected. Right now, I believe the array would be empty
api/controllers/v1/slack.ts
Outdated
| @@ -65,8 +80,9 @@ export const DisplayRepoOptions = async (event: APIGatewayEvent): Promise<APIGat | |||
| async function deployRepo(deployable: Array<any>, logger: ILogger, jobRepository: JobRepository, jobQueueUrl) { | |||
| try { | |||
| await jobRepository.insertBulkJobs(deployable, jobQueueUrl); | |||
| console.error('testing console error logging'); | |||
There was a problem hiding this comment.
Yup, it will be- left it in for Matt to reference regarding his comment on logging !
|
Hey @anabellabuckvar, I think there may be a bug with the test deployment process in that some jobs seem to fail due to an authorization error. I was looking into it and I wonder if we need to update the throwIfUserNotEntitled function such that it bypasses the check for admin users. For example, I don't have |
Stories/Links:
DOP-4599
Notes
This PR accomplishes two goals:
The PR is currently in PrePrd for reviewing purposes so reviewers can test themselves.
README updates