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 docker tag logic #15889

Closed
wants to merge 11 commits into from
Closed

Change docker tag logic #15889

wants to merge 11 commits into from

Conversation

pat-s
Copy link
Member

@pat-s pat-s commented May 16, 2021

fixes #15622

  • :latest -> :dev
  • :latest = :1 = :stable
  • Update docs

@techknowlogick @sapk

I could not find where the :1 tag is defined on releases. Is it only triggered by a local heuristic on release?

This change should change HEAD version tag to :dev, unless I overlooked something.

Feedback/help welcome :)

@codecov-commenter
Copy link

Codecov Report

Merging #15889 (1eb21d9) into main (58646ca) will decrease coverage by 0.00%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #15889      +/-   ##
==========================================
- Coverage   44.02%   44.01%   -0.01%     
==========================================
  Files         681      681              
  Lines       82333    82333              
==========================================
- Hits        36244    36237       -7     
- Misses      40186    40194       +8     
+ Partials     5903     5902       -1     
Impacted Files Coverage Δ
models/unit.go 46.57% <0.00%> (-2.74%) ⬇️
modules/log/event.go 59.90% <0.00%> (-1.89%) ⬇️
models/gpg_key.go 53.51% <0.00%> (-0.57%) ⬇️
services/pull/pull.go 43.83% <0.00%> (+0.45%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 41136db...1eb21d9. Read the comment docs.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label May 16, 2021
@lafriks
Copy link
Member

lafriks commented May 16, 2021

I don't like changing the meaning of tag. I would probably than prefer dropping it if we go this way

@6543 6543 added this to the 1.15.0 milestone May 16, 2021
@6543
Copy link
Member

6543 commented May 16, 2021

well we need consensus ... and more people testing gitea - especially windows users !

@techknowlogick
Copy link
Member

I could not find where the :1 tag is defined on releases. Is it only triggered by a local heuristic on release?

:1 comes from the manifest plugin, the logic is if there is a semver-like tag, then it'll split it up into 1, 1.4, 1.4.0 (assuming a non-RC/alpha tag), if it is an RC/alpha tag then it'll just use the exact tag (found here: https://github.com/drone-plugins/drone-manifest/blob/master/tagging/tagging.go#L21-L56)

I don't like changing the meaning of tag. I would probably than prefer dropping it if we go this way

the only meaning we are changing is latest, and that can't exactly be dropped.

@pat-s
Copy link
Member Author

pat-s commented May 17, 2021

and more people testing gitea - especially windows users !

That would be great ofc, but these people should be "testers" because they want to, not because they accidentally used :latest and then find themselves in a DB downgrading problem.
This happen(ed) quite often and is really a pain for everyone (users and contributors helping to downgrade).

:1 comes from the manifest plugin, the logic is if there is a semver-like tag, then it'll split it up into 1, 1.4, 1.4.0 (assuming a non-RC/alpha tag), if it is an RC/alpha tag then it'll just use the exact tag (found here: https://github.com/drone-plugins/drone-manifest/blob/master/tagging/tagging.go#L21-L56)

Thanks, I will have a look how I can integrate :latest and :stable into it.

@6543
Copy link
Member

6543 commented May 17, 2021

@pat-s ... good point

We can switch this schema each time migration level is equal on main and release branch ... so on each majour release

@lafriks
Copy link
Member

lafriks commented May 20, 2021

Problem is that we will pretty much break all instances with this change using :latest as they won't start anymore as essentially we will force downgrade on them. Also in all recommendations it is warned not to use :latest because of multiple reasons (cache, best practices etc)

@techknowlogick
Copy link
Member

@lafriks we'd wait until next major release to switch the :latest label, so that no downgrades would be possible.

@lafriks
Copy link
Member

lafriks commented May 21, 2021

I would propose :latest tag builds show This is development Gitea version. It's not recommended to be used on production environment. More info on top of every page as warning (like one on try.gitea.io). Where on more info would display popup on what version be used and how to disable this warning by adding HIDE_DEVELOPMENT_WARNING=true in app.ini

Also this would be displayed only for ci built main branch releases

@pat-s
Copy link
Member Author

pat-s commented May 21, 2021

I would propose :latest tag builds show This is development Gitea version. It's not recommended to be used on production environment. More info on top of every page as warning (like one on try.gitea.io). Where on more info would display popup on what version be used and how to disable this warning by adding HIDE_DEVELOPMENT_WARNING=true in app.ini

This could be done in addition to the new :dev tag. However I guess people opting in actively are probably just annoyed by this without a real benefit.

The issue that people end up unwanted in the dev build can IMO only be solved by switching the tag logic away from :latest.

@lafriks
Copy link
Member

lafriks commented May 23, 2021

My proposal would be to drop :latest

@pat-s
Copy link
Member Author

pat-s commented May 24, 2021

We could also get rid of :latest to have a more tidy tag setup. I don't have a strong opinion on this. However, I think that this would cause more disruption than refocusing to the stable releases because people would not get any updates anymore starting the next day (assuming auto-updates).

If we do this, we want to communicate this upfront throughout the period of a point release?

@techknowlogick
Copy link
Member

I have a strong opinion against getting rid of :latest because it is the default tag when pulling an image, and I believe it would cause more confusion than switching to stable.

@lafriks
Copy link
Member

lafriks commented May 26, 2021

At least it would force to specify one 🤣

@pat-s
Copy link
Member Author

pat-s commented Jun 2, 2021

@techknowlogick I've reduced the build matrix in this PR to only run the docker dry run pipelines temporarily.
I've also added a dry run for the manifest runs.

However, the manifest runs only get triggered by a tag / need a tag for correct image tagging.
I cannot push tags and was wondering if you/someone else could trigger a test run with a fictional tag (e.g. 99.99.99) to check if everything works? Not sure if this is possible in a PR from a forked repo though.

Maybe there is an easier way to test this which I am currently overlooking?

Also I think the :latest -> :dev should already be fine and the release tagging is still WIP.

@pat-s
Copy link
Member Author

pat-s commented Jun 5, 2021

Maybe @zeripath can also shine light on this? 🙂

@zeripath
Copy link
Contributor

Maybe @zeripath can also shine light on this?

OK, I think we need to balance the benefits and risks of our current system in toto:

  1. We have very few people who build gitea and test gitea latest deliberately. It's highly likely that the vast majority of our bug reports regarding breaks in main come from docker users who - unwittingly or not - are running latest and don't realise that this is unreleased.
  2. Changing latest to match the last released version would fix that confusion but it would seriously slow down testing because of the loss of this otherwise large testing pool.
  3. This would mean that migration bugs would only be spotted on release - which would mean that more people would be affected by them.
  • However, we now have migration tests and our dumping and restore tool is better - so that's somewhat ameliorated.
  1. It would mean that 1.x.0 releases would have more bugs in them at the start.
  • This would mean that developer load for dealing with release bugs could be worse - i.e. on release day we're going to have to be prepared to fix a flurry of bugs in a quick 1.x.1 release. (Which we already face.)
  • And we may have to spend more time backporting. (Looking at git rev-list --count origin/release/v1.x...v1.1x.0 reveals that we currently have 119 backports on to 1.14 before we've even released 1.15. By the end of 1.13 we had 130, 1.12 was 112, 1.11 was 124 and 1.10 was 66. So our backport load is already quite high and increasing.)
  1. Conversely the lack of apparent feature release would lead to more interest in getting a release out of the door.
  • Perhaps this would lead to more people trying the RCs as they'd be more important to them?

In the end though I'm tired of being flamed by (often novice) docker users - whilst I don't think we're doing anything wrong and it wasn't wrong originally - I think consensus has moved underneath us and most people on dockerhub have moved to ":latest" to represent the latest stable.

So unfortunately I think we probably do need to do it - but in doing so we're going to have acknowledge that there will be consequences on our ability to get things tested and likely we're going to have to change the way we manage releases and merging and in particular refactors.

@6543
Copy link
Member

6543 commented Jun 25, 2021

If we go this way, we should add a new tag "main" who is the new "latest" and realy encurage users to run this if they like to support us!

@6543
Copy link
Member

6543 commented Jun 25, 2021

"dev" or "testung" sounds to unstable ...

@zeripath
Copy link
Contributor

main then

@techknowlogick
Copy link
Member

as much as I love to yak shave (I don't), we need to denote some level of "this isn't the stable release" so main could impart that it is the "main" image and it should be used over some other image. Images such as minio use edge for that, but I find dev/develop just as suitable.

@pat-s
Copy link
Member Author

pat-s commented Jun 29, 2021

I'd also favor dev over main for the reasons outlined by @techknowlogick

However I would still need some feedback how I/we can test drive this. See my last comment 🙂

@6543
Copy link
Member

6543 commented Jul 10, 2021

if we go this direction I think it's time to make this pull ready

@techknowlogick
Copy link
Member

Closing in lieu of #16421

@techknowlogick techknowlogick removed this from the 1.15.0 milestone Jul 13, 2021
@go-gitea go-gitea locked and limited conversation to collaborators Oct 19, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Docker: re-evaluate tag heuristic (:latest, :dev)
7 participants