Outlier semver detection#20
Conversation
| // SemverOutlierMajorVersionThreshold defines the number of major versions that must be skipped before | ||
| // the next version is considered an outlier | ||
| // setting this to 1 allows NO major versions to be skipped | ||
| SemverOutlierMajorVersionThreshold = 1 |
There was a problem hiding this comment.
IMO this should default to two
I know there are legitimate repos that have skipped a major version before
| }, | ||
| }, | ||
| { | ||
| name: "no outliers", |
There was a problem hiding this comment.
this does not seem like a test case with no outliers
There was a problem hiding this comment.
and in fact we don't have a no outliers test
so maybe add a test case that has major versions 1, 2, 3 and 4, with no outliers expected?
There was a problem hiding this comment.
another thing to test - 1, 2, 5, 9
Are both 5&9 detected as outliers?
Similarly - 4, 5, 6
presumably there shouldn't be any outliers there
And if we know the current semver, we should use that as the start - so that if we're on version 5, we shouldn't report 6 as an outlier just because the total set of detected major versions was 1, 2, 5, 6, 7
| assert.Equal(t, test.expectedOutliers, actualOutliersStrings) | ||
| assert.Equal(t, test.expectedRemaining, actualRemainingStrings) |
There was a problem hiding this comment.
I think these should use req.Equal
but unimportant
laverya
left a comment
laverya
left a comment
There was a problem hiding this comment.
overall looks like a good most-of-the-way solution
I'm still worried about a situation where the currently installed version is part of an "outlier" range though
2 participants
This fixes #19 but doesn't have a lot of error room.
Open to suggestions for better thresholds here.
The problem is summarized by: Elasticsearch has a lot of tags of their image (see list at the bottom of this message). It's very easy to split this into semver and non-semver. But what is that 43.0.0 doing? It's clearly not really semver, and it will always report as a false "newer" version. So this PR looks for Major version gaps and discards anything after a gap.