-
Notifications
You must be signed in to change notification settings - Fork 879
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
[WTH Core] Create Github Workflow to build IntroToAKS docker images #284
base: master
Are you sure you want to change the base?
[WTH Core] Create Github Workflow to build IntroToAKS docker images #284
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.
I did not test this workflow. On manual review it LGTM. Blast radius small since it's manual and scoped to WTH001.
Two suggestions to add to this PR before merging:
- Document existence of this workflow in student guide, and who would want to use it and why.
- Document what a typical WTHer should do for lines 13 and 14, and amend "These four variables" to what looks like should be "These two variables" (REGISTRY and REPOSITORY) since the other env vars are set based on REPOSITORY.
Thanks for the review. I should have been clearer in the PR that purpose of this workflow is to make life easier for the WTH owners, not for students. (As I noted in the PR, it was inspired by the need to rebuild the base docker images for #280 & #202). So IMHO, I wouldn't make mention of this workflow to students at all. |
Makes sense, thanks. Approved and I will add another reviewer as two are required. |
Thanks @plzm! Please reach out if you need help customizing this once it's been merged. (as I wrote in the edits, it will need customization in order to run once you've merged it in) |
Thanks @larryclaman. Looks like lines 13 and 14. We've already started talking on the team about a workflow trigger strategy that would make sense with all the distinct WTHs, including branching etc. Likely I expect we'd trigger this workflow off a merge to master so that updated images are built then. When we have a default registry and repository we'll update them in the script. |
@plzm I haven't reviewed this code yet or tested it. As per Larry's comment. The intention for this action would be that it is an administrative tool that is manually triggered by the repo maintainer anytime we update the code of the sample app from the "001-IntroToKubernetes" hack. I wouldn't auto-run this on merges to master as it would not be needed unless the sample app code in 001 is changed. IIRC, we have a limited # of 'free' actions we can run. We don't need to abuse that. Its goal is to build a new Docker container of the sample app and push it to the WTH repository on Dockerhub. To do this, the action would need a secret (the login/pwd) for our Dockerhub account. I have those secrets and can work with you offline to load them into GH and test the action. The secrets are referenced on lines 36 & 37. Let's not merge this PR until that testing is complete. |
Makes sense. I converted the PR to draft as an additional gate to merging before we address this. |
This pr introduces a github actions workflow that will help automate (re)building container images for use by the IntroToAKS hack.
The PR was inspired by issue #202 and PR #280.
As currently implemented, the action will only run manually (using workflow_dispatch), but if desired it could be improved to automatically rebuild images if new code is checked in.
The repo owners will need to customize some of the variables (see comments inline) used by this workflow; specifically the repo locations on dockerhub (which could be migrated to github if desired)