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

change(release): Make the latest tag point to the production build, rather than the build with experimental features #7817

Merged
merged 12 commits into from Nov 2, 2023

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented Oct 25, 2023

Motivation

We want to remove redundant DockerHub tags, and fix incorrect tags.

Close #7415

Specifications

https://github.com/docker/metadata-action#flavor-input

Solution

  • Stop publishing the v1.x.y tag for future releases
  • Stop trying to publish the edge tag (it never actually got published) it only gets published in CI on main branch builds, which is what we want
  • Make latest point to the production image, not experimental
  • Make overlapping prod/experimental tags point to the production image
  • Add a deprecation warning and its section to the changelog

Related Cleanups:

  • Rearrange some changelog items
  • Remove an OS CI workflow trigger when the docker build workflow changed

Testing

We won't be able to test this until the release.

The expected behaviour is in the tag workflow comments:
https://github.com/ZcashFoundation/zebra/pull/7817/files#diff-5a5111df219975ea6b3b69567752b89fa0c670d5a67598674ad411da2ed96985R93-R102

And it should appear here:
https://hub.docker.com/r/zfnd/zebra/tags

Review

This should get merged this week because it blocks the release.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@teor2345 teor2345 added A-devops Area: Pipelines, CI/CD and Dockerfiles C-cleanup Category: This is a cleanup P-Medium ⚡ I-usability Zebra is hard to understand or use A-release Area: Zebra releases and release management labels Oct 25, 2023
@teor2345 teor2345 requested a review from a team as a code owner October 25, 2023 02:01
@teor2345 teor2345 self-assigned this Oct 25, 2023
@teor2345 teor2345 requested review from oxarbitrage and removed request for a team October 25, 2023 02:01
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 25, 2023
@teor2345 teor2345 added C-removal Category: Features that have been removed and removed C-cleanup Category: This is a cleanup C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels Oct 25, 2023
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 25, 2023
@teor2345 teor2345 removed the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 25, 2023
@teor2345 teor2345 requested a review from a team as a code owner October 25, 2023 03:17
@github-actions github-actions bot added the C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG label Oct 25, 2023
Copy link
Member

@gustavovalverde gustavovalverde 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 deep-dive into uses: docker/metadata-action@v5 options, but can't this be accomplished without adding the extra build-experimental?

If possible, is there any extra complexity we're trying to avoid?

@teor2345
Copy link
Contributor Author

I did not deep-dive into uses: docker/metadata-action@v5 options, but can't this be accomplished without adding the extra build-experimental?

This change does not add an extra job, it makes the production build run after the experimental build.

Previously, the latest tag pointed to whichever job took longer, which is usually experimental because it compiles more code. Now the latest tag is only generated for the production build.

But I also changed the order just in case we add a tag with the same name in future.

(I forgot to use "needs" to make sure they run in the correct order, but that's fixed now.)

If possible, is there any extra complexity we're trying to avoid?

Is there a specific change you want to make to avoid complexity?

Removing the experimental build is out of scope, and we'll want an experimental build again as soon as we have some blockchain scanning in Zebra. So I'm not sure that's necessary. (Also it's a breaking change for anyone using experimental tags.)

@gustavovalverde
Copy link
Member

This change does not add an extra job, it makes the production build run after the experimental build.

Yeah, my bad. I just got confused with the GitHub diff

Is there a specific change you want to make to avoid complexity?

No, this is ok. I'll leave the final review to the assigned reviewer.

@teor2345 teor2345 removed the request for review from a team October 25, 2023 20:41
Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think that the current and proposed tags for this PR might be excessive when compared to the zcash docker image, which maintains a smaller set of tags, as seen here: https://hub.docker.com/r/electriccoinco/zcashd/tags.

My suggestion would be to simplify the tag structure. I propose the following tags:

  • latest
  • v1.4.0
  • v1.4.0-experimental

Here are my reasons for this recommendation:

  1. Minimizing Tag Count: Fewer tags make it easier to manage and navigate, reducing potential confusion.

  2. Avoiding Ambiguity: Using tags like 1.4 or 1 can lead to confusion.

  3. Clarifying sha-13230d0.experimental: If the purpose of tags like sha-13230d0.experimental is unclear, it's best to either rename them to something more descriptive or avoid publishing them.

  4. Prefixing with v: Prefixing version tags with v (e.g., v1.4.0 instead of 1.4.0) seems to be a common convention and looks better to me.

  5. Docker-Specific Information: Consider including Docker-specific details in the Docker Hub overview page instead of the README.

@teor2345
Copy link
Contributor Author

teor2345 commented Nov 1, 2023

I think that the current and proposed tags for this PR might be excessive when compared to the zcash docker image, which maintains a smaller set of tags, as seen here: hub.docker.com/r/electriccoinco/zcashd/tags.

My suggestion would be to simplify the tag structure.

I like the idea of this change, and I agree with the goal of simplifying things for users.

But it seems like a significant change that might be beyond the scope of the ticket or PR. It could also be a breaking change for some users.

To find out if it is a breaking change, we can check which tags are actually being used:
https://docs.docker.com/trusted-content/insights-analytics/#view-the-analytics-data

I don't have access to that data, but @gustavovalverde does, or he can give someone an account.

Then if it tag isn't used much, we can just delete it. If it is used, we can announce its deprecation in one release, and delete it in the next. That way users have time to change their scripts.

I also don't want to block the release on a longer investigation, so I suggest we make these changes in this PR:

  • Stop publishing the 1.x.y tag for future releases (this can't be a silently breaking change, because the tag never updates)
  • Announce the deprecation of the other tags we want to delete in the changelog
  • Make latest point to the production image, not experimental
  • Make overlapping prod/experimental tags point to the production image

@teor2345
Copy link
Contributor Author

teor2345 commented Nov 1, 2023

Actions from the meeting about this PR:

Minimise this PR.
Make a note in the changelog about proposed changes below (based on user feedback and docker image pull statistics).

Open tickets for:

Check docker pull statistics and consider deleting:

  • 1.x.y (use v1.x.y instead)
  • 1.x
  • 1
  • sha-xxxxxxx

And add:

  • latest-experimental

And Change:

  • .experimental to -experimental

Deleting release tags is blocked by #7680 so we can see release failures easily.

Changing features is blocked by changing the tags, because we want the new features to appear under the new tag names:

  • add all off-by-default features to the experimental build: elasticsearch, journald, prometheus, filter-reload
  • add an experimental feature to zebrad
  • add the experimental feature to the GitHub experimental feature variable, and the Docker builds/tests. Consider removing experimental from the OS builds because they are slow already.

Final set of tags:

  • latest
  • v1.x.y
  • latest-experimental
  • v1.x.y-experimental

@teor2345 teor2345 changed the title change(release): Stop publishing v1.x.y DockerHub tag, users should use 1.x.y (no v) instead change(release): Make the latest tag point to the production build, rather than the build with experimental features Nov 1, 2023
@github-actions github-actions bot added the C-feature Category: New features label Nov 1, 2023
CHANGELOG.md Outdated Show resolved Hide resolved
@teor2345
Copy link
Contributor Author

teor2345 commented Nov 2, 2023

The new tickets are #7891 and #7892, and this PR is minimal.

Copy link
Contributor

@oxarbitrage oxarbitrage left a comment

Choose a reason for hiding this comment

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

I think this is good enough for now, thank you.

mergify bot added a commit that referenced this pull request Nov 2, 2023
@mergify mergify bot merged commit a1e476e into main Nov 2, 2023
138 checks passed
@mergify mergify bot deleted the dockerhub-tags branch November 2, 2023 19:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-devops Area: Pipelines, CI/CD and Dockerfiles A-release Area: Zebra releases and release management C-feature Category: New features C-removal Category: Features that have been removed C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG I-usability Zebra is hard to understand or use
Projects
None yet
Development

Successfully merging this pull request may close these issues.

devops: Remove redundant tags in Docker Hub
3 participants