search

fluxcd / flux

Successor: https://github.com/fluxcd/flux2
https://fluxcd.io
Apache License 2.0
6.9k stars 1.08k forks source link

Use longer slice of Revision hash for ShortRevision #3519

Closed kingdonb closed 2 years ago

kingdonb commented 3 years ago

Fix #3517

There have been several reports from groups with 10,000 commits or more in their repo that fluxctl has trouble when the Git SHA has a collision, because only the first 7 characters are used.

For future-proofing, I chose 12 characters. I bet 10 would be enough. We could do the birthday problem calculation to decide on an appropriate number, I just want to see if tests still all pass when I've fixed all the instances of that [:7] that I could find throughout the codebase, or if there might be one hidden in a dependency somewhere outside the project.

kingdonb commented 3 years ago

Depending on core maintainer availability for reviews, and our disposition toward this change as PATCH or MINOR, this could be released as early as Monday, in 1.23.2, or some time after that in 1.24.0.

To say it another way, I have it marked as 1.24.0 milestone for now, but this could change.

kingdonb commented 3 years ago

We can consider this for including in Flux v1.24.0

I do not know if it would be considered a breaking change, it is clearly a breaking change between the fluxctl CLI and the flux daemon, but I think users are expected to upgrade their fluxctl binary when they upgrade flux, so I'm not sure if that means it should be excluded.

It's not a particularly urgent bug for us to fix, the workaround is quite simple but also a bit ugly. (Just push another commit that does not have a "birthday neighbor" as explained in #3517, so nobody is waiting desperately for this to be released as a fix.)

I'm OK letting it sit for a while, I have not marked it for the 1.24.0 milestone, but if we are going to merge this we probably will want it to be on a MINOR release even if it is a bug fix, so that people will at least likely remember to upgrade fluxctl.

kingdonb commented 3 years ago

Per our discussion on #3517 this PR is deliberately not included in 1.24.0

This will be the only PR left 🕯️ on the Flux v1 repo, assuming everything else that remains in the 1.24.0 milestone can get approved for including and merged for the next release as planned 🎉

pjbgf commented 2 years ago

This project is in Migration and security support only, so unfortunately this PR won't be merged. We recommend users to migrate to Flux 2 at their earliest convenience.

More information about the Flux 2 transition timetable can be found at: https://fluxcd.io/docs/migration/timetable/.