Docker release CT-3 #4616
Docker release CT-3 #4616
Conversation
… update-dockerfile
|
It's going to take a lot of additional context gathering for me to give a useful review on this PR. In the interest of time, I'm going to pull myself off as a reviewer but if you're in a pinch later you can add me back and I'll see what I can do. |
| # Github package 'latest' tag wrangler for containers | ||
| ## Usage | ||
|
|
||
| Plug in the necessary inputs to determine if the container being built should be tagged 'latest; at the package level, for example `dbt-redshift:latest`. |
There was a problem hiding this comment.
Could you add some more about latest vs minor_latest? I want to make sure we capture why we need both tags and not just 1 (I could see this coming up as a question from a new hire or somebody outside of the team)
There was a problem hiding this comment.
TBH, I'm not 100% sure why we need the minor_latest beyond the idea that we might have breaking changes between a 1.0.latest and a 1.1.latest? @jtcohen6 would be a better person to answer that question.
There was a problem hiding this comment.
Sorry I missed this comment a few days ago! Rationale is to provide consistency with how we're thinking about patch + minor upgrades elsewhere (e.g. in dbt Cloud).
There are two premises after v1.0:
- Use patches as soon as they're available
- Minor releases won't have breaking changes (we've committed to this!), but they will touch significantly more code than patch releases
As such, we should let users decide when they want to perform the minor version upgrade. Each minor version will be officially supported for 12 months after its initial release.
So I wouldn't be surprised if some users want to stick with 1.0.latest, for a few weeks or a month, even after the 1.1.0 release. The inclusion of a 1.0.latest tag will give them the opportunity to coordinate upgrades at a time of their choosing. During that period, if we have to cut a "security" patch for v1.0 (security bug, dependency/installation issue), they'll still get it automatically, as they should.
There was a problem hiding this comment.
This all LGTM so thanks for incorporating the feedback. I'm going to approve but let's make sure @jtcohen6 addresses the last open comment. You can merge and always open another PR to clean-up but I just don't want to lose that discussion if we do
| @@ -0,0 +1,20 @@ | |||
| name: "Github package 'latest' tag wrangler for containers" | |||
There was a problem hiding this comment.
This is awesome! I created a place to put actions that can be shared across repositories. We should move this action over there at some point, I can see this being handy in many places. https://github.com/dbt-labs/actions
| package_request = requests.get( | ||
| f"https://api.github.com/orgs/dbt-labs/packages/container/{package}/versions", | ||
| auth=("", gh_token), | ||
| ) |
There was a problem hiding this comment.
This looks like it works as expected. This uses packages that have been released to github as the source of truth. If we wanted to use this action to determine the lastest version of a dbt python package, for example, we might get an incorrect answer if a Docker image doesn't exist yet for it. I don't think it should be part of this PR, but what are your thoughts on making this action a little more generic and using PyPI as the source of truth? There is some prior art from the ole release process get this info.
https://github.com/dbt-labs/dbt-release/blob/b6efbb06cc52b84f6f935350046a737e9ad76c38/scripts/release-pypath/builder/common.py#L210-L222
https://github.com/dbt-labs/dbt-release/blob/b6efbb06cc52b84f6f935350046a737e9ad76c38/scripts/release-pypath/builder/common.py#L62-L75
There was a problem hiding this comment.
I'd rather leave docker dependent only on what docker images already exist, but I hold that opinion very loosely. In the long term I'd actually like to use GH releases as the source of truth for this, but it won't make sense to do that until we have a more complete release system (as opposed to this work which was meant to only address how Docker gets released)
| latest: | ||
| description: "Wether or not built container should be tagged latest (bool)" | ||
| minor_latest: | ||
| description: "Wether or not built container should be tagged minor.latest (bool)" |
There was a problem hiding this comment.
General question.. I've never known what to use as return types for GHA & scripting things. This returns True | False, but does it make sense to return 0 | 1 or other bool representations?
There was a problem hiding this comment.
Ok, so this is something that took a while to sort out, but the TL;DR is that it doesn't matter. GHA is all written in javascript, which types everything dynamically so any variable you set as an output operates in the same way.
I've defaulted to using string representations by default because all values coming from bash are untyped and treated as strings. It's one less thing to have to remember. :)
* new docker setup * formatting * Updated spark: support for extras * Added third-party adapter support * More selective lib installs for spark * added docker to bumpversion * Updated refs to be tag-based because bumpversion doesn't understand 'latest' * Updated docs per PR feedback * reducing RUNs and formatting/pip best practices changes * Added multi-architecture support and small test script, updated docs * typo * Added a few more tests * fixed tests output, clarified dbt-postgres special case-ness * Fix merge conflicts * formatting * Updated spark: support for extras * Added third-party adapter support * More selective lib installs for spark * added docker to bumpversion * Updated refs to be tag-based because bumpversion doesn't understand 'latest' * Updated docs per PR feedback * reducing RUNs and formatting/pip best practices changes * Added multi-architecture support and small test script, updated docs * typo * Added a few more tests * fixed tests output, clarified dbt-postgres special case-ness * changelog * basic framework * PR ready excepts docs * PR feedback automatic commit by git-black, original commits: d6cc8b3
resolves #CT-3
Description
Github actions to release a matrix of docker images to github packages and handle tagging/untagging of latest/ minor.latest versions.
Checklist