-
Notifications
You must be signed in to change notification settings - Fork 72
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
DOP-4599: Deploy-all slack button deploys all active versions of repos #1042
base: main
Are you sure you want to change the base?
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. |
.github/workflows/deploy-stg-ecs.yml
Outdated
@@ -3,6 +3,7 @@ on: | |||
branches: | |||
- "main" | |||
- "integration" | |||
- "DOP-4599" |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we still log the error? It can sometimes be such a blessing.
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.
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we remove this?
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.
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