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

Publish latest-tagged images when building releases #2660

Merged
merged 2 commits into from
Apr 14, 2022

Conversation

kevin-bates
Copy link
Member

What changes were proposed in this pull request?

When using the Makefile targets to build and publish images for each release, we were not equating the built images with a latest tag. As a result, this needed to be a manual step that, often, fell through the cracks. This pull request applies the latest tag to the corresponding image when releases are being built (i.e., when the Makefile variable TAG does not equal "dev"), and pushes those tagged images to the respective registry.

How was this pull request tested?

This was tested by temporarily prefixing the docker push commands with echo since I didn't want to update any hash values relative to the images already present. This test ensured that the if [ "$(TAG)" != "dev" ]; statement was executed correctly. I also used make -n publish-container-images and make -n TAG=3.7.0 publish-container-images to visually see the results - which only echos the logic flow although one can't really ensure the if statement is properly executed (thus the echo-based test).

Resolves: #2659

Developer's Certificate of Origin 1.1

   By making a contribution to this project, I certify that:

   (a) The contribution was created in whole or in part by me and I
       have the right to submit it under the Apache License 2.0; or

   (b) The contribution is based upon previous work that, to the best
       of my knowledge, is covered under an appropriate open source
       license and I have the right under that license to submit that
       work with modifications, whether created in whole or in part
       by me, under the same open source license (unless I am
       permitted to submit under a different license), as indicated
       in the file; or

   (c) The contribution was provided directly to me by some other
       person who certified (a), (b) or (c) and I have not modified
       it.

   (d) I understand and agree that this project and the contribution
       are public and that a record of the contribution (including all
       personal information I submit with it, including my sign-off) is
       maintained indefinitely and may be redistributed consistent with
       this project or the open source license(s) involved.

@kevin-bates kevin-bates added component:build build and build related issues(dependencies and docker) component:docker Container/Docker images related issues labels Apr 13, 2022
@kevin-bates kevin-bates added this to the 3.8.0 milestone Apr 13, 2022
@elyra-bot
Copy link

elyra-bot bot commented Apr 13, 2022

Thanks for making a pull request to Elyra!

To try out this branch on binder, follow this link: Binder

@ptitzler
Copy link
Member

ptitzler commented Apr 13, 2022

The reason why we didn't do this in the past is that not every dev release should be tagged latest.

Problem scenario:

  • current version 3.7 (latest)
  • we need to publish version 3.6.x (or 3.5.x, ...) to fix a major issue

latest should always refer to to most current release, not the last recently updated release because it could implicitly cause users to switch between major or minor versions.

@kevin-bates
Copy link
Member Author

kevin-bates commented Apr 13, 2022

Good point @ptitzler. IIRC we had decided to not publish latest tags at all, but it seems that's water under the bridge at this point.

Would a check that the last digit of the version (TAG) is 0 (zero) be sufficient? This would "break" if we were to introduce a brand new minor release after a major release. E.g/ 3.7.0, 4.0.0, 3.8.0 (breaks). But I would think we'd only apply patch releases on older versions following the introduction of a major release.

EDIT: This doesn't address "latest" patch releases - duh!

@akchinSTC
Copy link
Member

I misunderstood what others were saying earlier. The release script doesn't publish the container images, that is done manually post-python package release and push to pypi.
With that said, I don't think we need to make any changes here. This PR will only update the Makefile on master, which will update the tag accordingly since it will always be the 'latest' release.
Existing maintenance branches, since they will all have their own respective Makefiles without this update, will continue to publish using their publish targets without ever updating the latest tag, which is what we want.

@ptitzler
Copy link
Member

ptitzler commented Apr 13, 2022

Existing maintenance branches, since they will all have their own respective Makefiles without this update, will continue to publish using their publish targets without ever updating the latest tag, which is what we want.

Unfortunately this will not be true after version 3.8 was released and the problem still persists:

  • release 3.8 with this change
  • release 3.9
  • release 3.8.x -> breaks latest

@kevin-bates
Copy link
Member Author

I think we need to incorporate the branch from which the release is being built and assuming that only 'latest" releases are built from the master branch.

release 3.8.x -> breaks latest

This will occur on a branch other than master. As a result, I'm adding a Makefile variable LATEST_RELEASE:=0 that, when 1 will trigger the tagging and publication of the latest tags. The release script that modifies the Makefile TAG variable, will also, conditionally based on git_branch, modify the LATEST_RELEASE variable. This form of the Makefile will then be tagged with the release and checked into GIT under that tag. Then, once the release has been pushed to PyPI, the "release operator" will run make publish-container-images using this tagged form of the Makefile which will appropriately trigger the tagging and publication of the latest images.

@ptitzler
Copy link
Member

ptitzler commented Apr 13, 2022

I like the idea! If we can, we should avoid the need to patch the Makefile, as that might have an unintended side-effect if the "release operator" commits the Makefile changes.

git rev-parse --abbrev-ref HEAD git branch --show-current will give us the current branch name and could be evaluated in the Makefile itself to determine whether we are on master and need to do extra work.

@kevin-bates
Copy link
Member Author

we should avoid the need to patch the Makefile

I agree, but that's what already happens so this fits the current process. I'd rather not introduce a process change in this PR - especially since I figured it would be a 5-minute effort - which is clearly not the case.

Since you're proposing placing this in the Makefile, that seems fine. I'd rather not change the current "patching" of the Makefile. I'll go ahead and revert the release script changes, adding git branch --show-current to the Makefile publish targets.

Copy link
Member

@ptitzler ptitzler left a comment

Choose a reason for hiding this comment

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

LGTM based on code review.

@akchinSTC akchinSTC merged commit 1b67ea7 into elyra-ai:master Apr 14, 2022
@kevin-bates kevin-bates deleted the publish-latest-images branch April 19, 2022 16:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component:build build and build related issues(dependencies and docker) component:docker Container/Docker images related issues
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Dockerhub elyra/kf-notebook:latest is 3.6.0 instead of 3.7.0
3 participants