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

[WTH Core] Create Github Workflow to build IntroToAKS docker images #284

Draft
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

larryclaman
Copy link
Contributor

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)

@jrzyshr jrzyshr requested a review from plzm November 9, 2021 22:48
Copy link
Contributor

@plzm plzm left a 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:

  1. Document existence of this workflow in student guide, and who would want to use it and why.
  2. 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.

@larryclaman
Copy link
Contributor Author

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.

@plzm
Copy link
Contributor

plzm commented Nov 16, 2021

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.

@larryclaman
Copy link
Contributor Author

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)

@plzm
Copy link
Contributor

plzm commented Nov 16, 2021

Makes sense, thanks. Approved and I will add another reviewer as two are required.

Thanks @plzm Patrick El-Azem FTE! 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.

@jrzyshr
Copy link
Collaborator

jrzyshr commented Nov 17, 2021

@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.

@plzm plzm marked this pull request as draft November 22, 2021 11:07
@plzm
Copy link
Contributor

plzm commented Nov 22, 2021

@plzm Patrick El-Azem FTE 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.

@jrzyshr jrzyshr changed the title Create Github Workflow to build IntroToAKS docker images [WTH Core] Create Github Workflow to build IntroToAKS docker images Jun 8, 2022
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.

3 participants