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

automation/deployer: respect PR label deployer:skip-deploy #4065

Merged
merged 4 commits into from
May 16, 2024

Conversation

consideRatio
Copy link
Contributor

@consideRatio consideRatio commented May 13, 2024

I wanted this a lot in order to:

  1. Not need to press merge and then quickly manually cancel the triggered workflow when deciding to not deploy/test
  2. Not create noise in #gha-failures for manually cancelled workloads, and to not need to comment that it was just a intentionally cancelled workload
  3. Be able to visibilize a deploy skip is planned
  4. Not incurr delays by queueing many deploys when only a one or few may be relevant

I also wanted this for some additional reasons, like reducing cloud costs for communities and reducing risk of #3214 further, but the main reasons motivating me now to implement this feature was those above.

Review notes

  • About testing of changes
  • About outdated deploy PR comments
    The PR comment made listing what gets deployed won't get updated once the deployer:skip-deploy label gets added. This is also the case if you first push a commit that leads to a deploy, and then reverts the commit. There is no logic to delete an existing comment or update it to declare nothing will get deployed in those situations.

@consideRatio consideRatio changed the title deployer: respect label 'deployer:skip-deploy' deployer: respect PR label 'deployer:skip-deploy' May 13, 2024
@consideRatio consideRatio marked this pull request as draft May 13, 2024 21:08
@consideRatio consideRatio force-pushed the pr/deployer-skip-deploy branch from d8c70de to 80ef99b Compare May 13, 2024 21:09
@2i2c-org 2i2c-org deleted a comment from github-actions bot May 13, 2024
@consideRatio consideRatio added the deployer:skip-deploy Skips deployment of anything (support, staging, prod) label May 13, 2024
@2i2c-org 2i2c-org deleted a comment from github-actions bot May 13, 2024
@consideRatio consideRatio reopened this May 13, 2024
@consideRatio consideRatio marked this pull request as ready for review May 13, 2024 21:16
@consideRatio consideRatio requested a review from yuvipanda May 13, 2024 21:36
Comment on lines -141 to +148
f"support-and-staging-matrix-jobs={json.dumps(support_and_staging_matrix_jobs)}"
f"support-and-staging-matrix-jobs={json.dumps(support_and_staging_matrix_jobs)}\n"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is an PR unrelated fix - when we write to the file path $GITHUB_ENV, it should always get full lines written to it. Otherwise we could get lines like MY_ENV=testSOMEOTHER_ENV=value

@consideRatio
Copy link
Contributor Author

consideRatio commented May 13, 2024

@yuvipanda I requested your review since you were involved in discussion about this in #2009 - focusing on the work patterns it leads to for 2i2c engineers rather than the technical implementation, is this an acceptable change to you?

EDIT: From a video meet with Yuvi, I understand no objection is raised.

@consideRatio consideRatio changed the title deployer: respect PR label 'deployer:skip-deploy' automation/deployer: respect PR label 'deployer:skip-deploy' May 13, 2024
@consideRatio consideRatio changed the title automation/deployer: respect PR label 'deployer:skip-deploy' automation/deployer: respect PR label deployer:skip-deploy May 13, 2024
@consideRatio consideRatio requested review from a team and removed request for yuvipanda May 15, 2024 09:18
Copy link
Member

@GeorgianaElena GeorgianaElena left a comment

Choose a reason for hiding this comment

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

@consideRatio, sorry if it's a silly question, I didn't spent much time reasoning about it, but why have the label special cased in the deployer and not inside the workflow itself, maybe similar to

jobs:
update-project-board-fields:
if: github.event.label.name == 'support'
runs-on: ubuntu-latest
steps:
?

@consideRatio
Copy link
Contributor Author

@consideRatio, sorry if it's a silly question, I didn't spent much time reasoning about it, but why have the label special cased in the deployer and not inside the workflow itself, maybe similar to

jobs:
update-project-board-fields:
if: github.event.label.name == 'support'
runs-on: ubuntu-latest
steps:

?

The crux is that the information that is available in context under github... differ depending on how the workflow was triggered. The linked example above had a trigger of on.issues.types[labeled], and that has context of a single label for example -- the one label being added.

For pull_request triggered workflows, there are information about the pull request, including its labels, but for push triggered workflows, github can't even know for sure the push stemmed from a merged pull request, so it doesn't provide a context describing information about a pull request.

Due to that, I ended up adding a job step to get the associated PRs labels adjusting for the context on how to acquire it - I'll add a comment clarifying this next to the step.

@consideRatio consideRatio merged commit 74b3c4c into 2i2c-org:main May 16, 2024
40 checks passed
Copy link

🎉🎉🎉🎉

Monitor the deployment of the hubs here 👉 https://github.com/2i2c-org/infrastructure/actions/runs/9115204920

@consideRatio
Copy link
Contributor Author

I'll go for a merge here to not add overhead of thinking about the planning - I've performed self-review of this and think its acceptable to merge without further review by someone else.

@GeorgianaElena
Copy link
Member

@consideRatio, would you be willing to document this label and when to use it (maybe in our SRE section)?

I'm not suggesting to think up-front about all possible scenarios, but while you notice yourself using it, I think it would be very useful for others on the team to have it documented and to understand when it it's beneficial to skip the deploy.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
deployer:skip-deploy Skips deployment of anything (support, staging, prod)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

A way to merge and not trigger CD pipeline
2 participants