Conversation
| { | ||
| raw: "v23.2.0-cloudonly2", | ||
| phase: cloudonly, | ||
| phaseOrdinal: 2, |
There was a problem hiding this comment.
This is not a valid version, there must be a . after "cloudonly". This should not parse successfully.
(I think? What did the old code version do with this version string?)
There was a problem hiding this comment.
I think this matches the existing pattern? That accepts cloudonly with optional digits and no dot — it predates this PR. Do you know if these versions exist in the wild? If not, we could tighten the pattern to require a dot.
There was a problem hiding this comment.
Huh, you're right, and we have pre-existing test cases. My mistake! (I think as cloudonlys were being introduced there were some odd versions with inconsistent naming types; the set of tests in this package is based on a comprehensive review of the patterns of actual versions that existed in the CC control plane DB when I wrote this package, plus consultation with releng folks.)
version_test.go
Outdated
| } | ||
| } | ||
|
|
||
| func TestCompareTransitivitySmallSet(t *testing.T) { |
There was a problem hiding this comment.
Do the existing tests on sorting not already sufficiently prove these things? sorting uses Compare()
| regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly).(?P<phaseOrdinal>[0-9]+)-(?P<customOrdinal>(?:[1-9][0-9]*|0))-g[a-f0-9]+(?:-fips)?$`), | ||
| regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>alpha|beta|rc|cloudonly).(?P<phaseOrdinal>[0-9]+)-cloudonly(-rc|\.)(?P<phaseSubOrdinal>(?:[1-9][0-9]*|0))$`), | ||
| regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)-rc(?P<phaseOrdinal>[0-9]+)$`), | ||
| regexp.MustCompile(`^v(?P<year>[1-9][0-9]*)\.(?P<ordinal>[1-9][0-9]*)\.(?P<patch>(?:[1-9][0-9]*|0))-(?P<phase>cloudonly)(?P<phaseOrdinal>[0-9]+)?$`), |
There was a problem hiding this comment.
Here, I think? Is where cloudonly with no dot was allowed
Hoist compiled regexes and phase-name lookups to package level so they aren't rebuilt on every call. Fix two unescaped dots in parse patterns and a broken error message in the unknown-phase branch. Add tests and a benchmark.
|
tftr! |
2 participants
Hoist compiled regexes and phase-name lookups to package level so they aren't rebuilt on every call. Fix two unescaped dots in parse patterns and a broken error message in the unknown-phase branch. Add tests and a benchmark.