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

chore: only publish docker images for tagged versions #273

Merged
merged 1 commit into from Mar 1, 2023

Conversation

gilbsgilbs
Copy link
Contributor

@gilbsgilbs gilbsgilbs commented Mar 1, 2023

Previously, the following docker tags where created: - X.Y.Z when building a git tag matching vX.Y.Z - latest in any other case Starting from this commit, the following tags will be created: - For git tags matching X.Y.Z the following docker tags will be created: - latest - X - X.Y - X.Y.Z Other cases won't be tagged.


edit: old comment below, please ignore

Again, I was not able to test this commit end-to-end. Still, I tested that the script was generating an expected buildx command by manually specifying values for GITHUB_REF, GITHUB_SHA and IMAGE_NAME. It was tested with the following GITHUB_REFs:

  • refs/tags/v1.2.3 => stable, 1, 1.2, 1.2.3
  • refs/tags/v1.2.3-beta => 1.2.3-beta
  • refs/heads/master => latest, <GITHUB_SHA>
  • refs/heads/some-branch => <GITHUB_SHA> (even though this is not a possible case yet because the workflow events are only set for the master branch and tags)

@powerman
Copy link
Owner

powerman commented Mar 1, 2023

v0 is not "stable" by definition of semantic versioning.
Also, using tags like "stable" is a bad practice, because changes in major version imply some incompatible changes.
So, I'm against adding "stable" tag at all.

Also, for v0 using "X" is probably bad idea too, because it also imply incompatible changes. But to keep code simple we can ignore this and do not exclude "X" tag for v0 as a special case.

Main reason why I didn't bother adding such detailed tags was… dockerize is still v0 yet. :) I've nothing against adding them, but until v1 there is a little sense in using such tags.

BTW, if you're planning to continue work on dockerize I can add you as a collaborator (so you can work in same repo, not in a fork), but please setup your GPG key on GitHub and sign your commits if you want this.

fi
IMAGE_TAG="${{ secrets.CR_USER }}/$(basename ${GITHUB_REPOSITORY,,}):${TAG}"

PLATFORMS="$(IFS=, ; echo "${PLATFORMS[*]}")"
Copy link
Owner

Choose a reason for hiding this comment

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

Maybe my brain is too affected by using strong typing language like Go, but it hurts my eyes to see replacing array by string in same variable. Maybe it'll be better if you'll move this construction right to --platform/--tag args, without storing it in the variable at all?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. That was gross from me.

Copy link
Owner

@powerman powerman left a comment

Choose a reason for hiding this comment

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

Everything else at a glance looks good!

@gilbsgilbs
Copy link
Contributor Author

gilbsgilbs commented Mar 1, 2023

Thanks for the review.

v0 is not "stable" by definition of semantic versioning.

Gotcha. I don't really see a better naming though. Maybe latest could point to the latest git tag, and we could have a head or master tag that would point to the head of the master branch? WDYT?

Edit: I just implemented this proposal. Let me know what you think.

Also, using tags like "stable" is a bad practice, because changes in major version > imply some incompatible changes.
So, I'm against adding "stable" tag at all.

The same argument holds for latest tag, but it's still a fairly common practice to publish it (although arguably also a bad practice to use it). The thing is, I'm using very basic features of dockerize that are very unlikely to change across major versions. Moreover, I don't really care if it breaks, I'll just fix it as I'm not using dockerize in a critical application. In the meantime, I don't want to watch for dockerize releases just to keep me updated in the event of a major release, and I don't want to use head of master either which is generally deemed less stable.

I understand your argument though. I think the alternative I suggested above might be a good compromise, as it would only make the latest tag more stable, so only an improvement compared to the current situation.

Also, for v0 using "X" is probably bad idea too, because it also imply incompatible changes. But to keep code simple we can ignore this and do not exclude "X" tag for v0 as a special case.

That's coherent with your previous arguments. But I could counter-argue that someone using 0 tag should expect breaking changes as per semver, just like someone using latest should expect breaking changes.

Main reason why I didn't bother adding such detailed tags was… dockerize is still v0 yet. :) I've nothing against adding them, but until v1 there is a little sense in using such tags.

I wasn't aware of that. I thought this repo was stable because upstream dockerize is a pretty old and stable project now. [off topic]It might just be my own biases, because I despise this v0 rule from semver 😄. I think it makes the spec inconsistent and pointlessly surprising, while bringing very little value as pre-releases are more explicit and can be used instead. Not to mention it makes maintainers reluctant to move forward, and some maintainers are not even aware of this rule which just creates more confusion.[/off topic]

BTW, if you're planning to continue work on dockerize I can add you as a collaborator (so you can work in same repo, not in a fork), but please setup your GPG key on GitHub and sign your commits if you want this.

I'll consider that should I make other contributions. I'm unsure about it yet though.

@powerman
Copy link
Owner

powerman commented Mar 1, 2023

WDYT?

Latest is supposed to point to latest released (on docker hub) version. If we won't release untagged versions to docker hub at all then latest is for sure should point to one of vX.Y.Z. If we'll release untagged versions using git SHA to tag docker hub images then should latest point there or not is questionable, but personally I don't care and will be happy with latest points only to vX.Y.Z (actually I don't see any sense in releasing every untagged git commit to docker hub anyway).

The same argument holds for latest tag, but it's still a fairly common practice to publish it (although arguably also a bad practice to use it).

True. But this is common use case, and all linters are already handle it: they warn about using latest tag (or no tag at all which means latest) and force users replacing it with another, more concrete tag. Creating something latest-like but with different naming will just break such linter tools for no good reason.

I'm using very basic features of dockerize that are very unlikely to change across major versions.

This is very common use case, mine is mostly the same. :) But in this case you don't need to upgrade dockerize until something breaks or you'll need new feature.

And while outdated versions may, in theory, have security issues - even if you'll use "stable" or "latest" you don't get newer version until you'll rebuild your own container (and make sure you won't use already downloaded 2 years ago "latest" image from local docker cache which building your own container). Using "latest" everywhere and auto-rebuilding all your containers every few days "to be secure" usually won't work because of breaking changes here and there anyway.

So, the only viable solution today to get better security without too much hassle is to use "X" tag in case of v1+ or "0.Y" tag in case of v0.

I don't really care if it breaks

Then you can just use "latest".

I don't want to use head of master either which is generally deemed less stable

Actually it's not. Master is always a stable branch on my project, I never use it as a development/unstable branch. Of course, there is always a chance to merge something wrong/less tested, but it's an exception/mistake and usually master is as stable as any released version.

But I could counter-argue that someone using 0 tag should expect breaking changes as per semver, just like someone using latest should expect breaking changes.

You right, but in this case there is a latest tag to use, 0 tag isn't better than latests in any sense, so 0 is just redundant.

@powerman
Copy link
Owner

powerman commented Mar 1, 2023

I thought this repo was stable because upstream dockerize is a pretty old and stable project now.

Well, this repo was started as a fork, and my plan was to just provide few improvements to the original repo. But then #19 happens, which was a bad surprise to me. So, I detached repo and continue maintaining… just following existing versioning scheme, which happens to be v0.

Sure, it's stable. But releasing v1 means taking responsibility to avoid breaking changes, and before than I'd like to do some chores - review all the code, probably update/refactor it, and MAYBE make some incompatible changes to make CLI more consistent or prepared to future improvements. TBH I just don't have much spare time for this, plus tool "just works" and don't require much maintenance (so it don't catch my attention often enough), so I've no idea when I'll work on v1 release. Maybe it'll be v0 forever. :)

powerman
powerman previously approved these changes Mar 1, 2023
@gilbsgilbs
Copy link
Contributor Author

gilbsgilbs commented Mar 1, 2023

Of course, there is always a chance to merge something wrong/less tested, but it's an exception/mistake and usually master is as stable as any released version.

Yes, this was my concern. Using "latest-git-tag" (aka new latest) would be ideal for my use-case, but I guess using "latest-master" (aka current latest) would also be good-enough provided what you just said (and that's what I've done for the time being btw).


I don't have many things to add, I agreed with most of your points.

I can remove the GIT_SHA and master tags if you really insist on having latest tag pointing to the the latest uploaded tag. I usually don't like tagging each and every commit because that tends to create many dangling images that are almost never used and cannot be reclaimed, but I still made it because of this comment which suggested to me it was the author's intention at some point. I think having a master tag does not have the same overhead and could be useful for pre-release and testing purposes though, but one could as well build the image by themselves so I don't care much. I'll just do as you say :) .

@powerman
Copy link
Owner

powerman commented Mar 1, 2023

which suggested to me it was the author's intention at some point

Yeah, it was. But it quickly became clear it's useless, so it was commented out and leaved to be decided later. My bad, it should be deleted. :)

I've approved PR because I don't see anything wrong with current implementation, but, yes, personally I'd prefer to remove both "master" and git commit hash tags, and release only v-tags. Maybe support for v-tag which match not-just-numbers is a good feature, but it wasn't needed before and may be never needed, so probably it can be removed too. It's not hard to add if/when it'll be needed.

@gilbsgilbs
Copy link
Contributor Author

Done. Nice hedgehog by the way.

Previously, the following docker tags where created:

- `X.Y.Z` when building a git tag matching `vX.Y.Z`
- `latest` in any other case

Starting from this commit, the following tags will be created:

- For git tags matching X.Y.Z the following docker tags will be created:
  - `latest`
  - `X`
  - `X.Y`
  - `X.Y.Z`

Other cases won't be tagged.
@gilbsgilbs gilbsgilbs changed the title chore: publish versioned docker tags chore: only publish docker images for tagged versions Mar 1, 2023
@powerman powerman merged commit 7e45d86 into powerman:master Mar 1, 2023
6 checks passed
@powerman
Copy link
Owner

powerman commented Mar 1, 2023

Thanks!

@gilbsgilbs gilbsgilbs deleted the more-docker-tags branch March 1, 2023 19:19
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.

None yet

2 participants