Add Infrastructure Build and Deploy Workflows #167
Conversation
Why these changes are being introduced:
This commit adds the infrastructure deploy workflows to the this application,
asati. It adds new github automation, and updates the project's Makefile with
the new workflows. It also updates repo documentation.
How this addresses that need:
* Add dev build workflow
* Add stage build workflow
* Add prod promotion workflow
* Update Makefile w/ new workflows
* Add CODEOWNERS
* Update README.md
Side effects of this change:
None
Changes to be committed:
new file: .github/CODEOWNERS
new file: .github/workflows/dev-build.yml
new file: .github/workflows/prod-promote.yml
new file: .github/workflows/stage-build.yml
modified: Dockerfile
modified: Makefile
modified: README.md
Dockerfile
Outdated
| RUN pip install --no-cache-dir --upgrade pip pipenv | ||
|
|
||
| RUN apt-get update && apt-get upgrade -y && apt-get install -y git | ||
| RUN apt-get update && apt-get dist-upgrade -y && apt-get autoremove -y && apt-get install -y git |
There was a problem hiding this comment.
Since DataEng is building off the python:3.12-slim container, I don't think it's necessary to do the dist-upgrade here. I would leave any decisions regarding changes to the Dockerfile to the data engineers, especially @ghukill.
There was a problem hiding this comment.
I would recommend that they do a dist-upgrade and autoremove. The dist-upgrade will pick up changes related, through dependencies, to any packages that would be updated through an apt-get upgrade. This will help avoid ABI/API conflicts between packages and libraries. The autoremove command will uninstall packages that have been superseded during the upgrade or dist-upgrade process. This will save disk space (helping control our costs) and prevent either leaving insecure software installed in the container or a developer accidentally using software that has been replaced.
Performing our Dockerfile updates this way will bring us into line with the way AWS does their OS/Container updates. This would help prevent a situation where we could have containers that differ from our previous containers in installed software if we do a completely fresh build of a project or clone a project.
There was a problem hiding this comment.
This PR is supposed to be related just to the additional of the GitHub Actions workflows for publishing container images to AWS ECR, so any change to DataEng's Dockerfile feels out-of-scope for this PR.
Specific to this line in the Dockerfile, DataEng is using a pre-built Python container (python:3.12-slim), not a full Ubuntu distribution. So,the container is already highly optimized (see the Dockerfile that actually creates the source image here).
It does make sense to me to do apt-get upgrade to catch any out-of-date packages, but an apt-get dist-upgrade goes beyond what is necessary for the Python application to actually run.
The dependencies that matter for the Python application are all handled by pipenv install line below (since DataEng is tracking all their Python dependencies in the Pipfile in their application code).
There was a problem hiding this comment.
Thanks for raising this up @vab , and the detailed reponse @cabutlermit. I agree with Christopher's last three points about a specific image used, likely not needing to think/touch the underlying distribution, and that we spend quite a bit of time in our application code managing dependencies.
To my thinking -- and perhaps this is incorrect -- by tethering ourselves closely to the Docker image (and whatever updates it may receieve, knowing we get the updated form each CI build), we are effectively saying that our dependencies work with this particular image more than python 3.12 or Ubuntu 22.04 (arbitrary examples) more generally.
There was a problem hiding this comment.
@ghukill I have made the requested change. Can you approve this PR for merge? It will unblock me on some tickets. Thanks!
Why these changes are being introduced:
This commit makes a minor CODEOWNERS updated based on PR review.
Side effects of this change:
None
Changes to be committed:
modified: .github/CODEOWNERS
Pull Request Test Coverage Report for Build 13706817815Details
💛 - Coveralls |
Why these changes are being introduced:
This commit makes a minor update to the README.md file that removes this
repository's classification as an infrastructure repository.
Side effects of this change:
None
Changes to be committed:
modified: README.md
Why these changes are being introduced:
Reversion of Dockerfile requested in code review.
Side effects of this change:
None
Changes to be committed:
modified: Dockerfile
Why these changes are being introduced:
Revert changes to Dockerfile requested by code review.
Side effects of this change:
None
Changes to be committed:
modified: Dockerfile
Purpose and background context
This PR adds the infrastructure deploy workflows to the this application, dsc. It adds new github automation, and updates the project's Makefile with the new workflows.
How this addresses that need:
Side effects of this change:
None
Changes to be committed:
new file: .github/CODEOWNERS
new file: .github/workflows/dev-build.yml
new file: .github/workflows/prod-promote.yml
new file: .github/workflows/stage-build.yml
modified: Makefile
modified: README.md
How can a reviewer manually see the effects of these changes?
There will be no immediate effect. However, after the PR is merged with
main, future code updates should allow the running of the deployment workflows.Includes new or updated dependencies?
NO
Changes expectations for external applications?
NO
What are the relevant tickets?
https://mitlibraries.atlassian.net/browse/IN-1148
Developer
Code Reviewer(s)