Move prerelease compatibility code to common place, add unit tests #892
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsExpand to view the GitHub comments
To re-run your PR in the CI, just comment with:
|
| { | ||
| Title: "prerelease package with experimental release flag prerelease search", | ||
| Filter: Filter{ | ||
| PackageName: "mysql", | ||
| Prerelease: true, | ||
| }, | ||
| Expected: []filterTestPackage{ | ||
| // FIXME: This package should be returned. | ||
| // {Name: "mysql", Version: "0.9.0"}, | ||
| }, | ||
| }, |
There was a problem hiding this comment.
Just checking some curls,
Packages with versions 0.x.y are just returned by the API if experimental is set to true, but they should be returned because of prerelease=true, isn't it?
$ curl -s "https://epr.elastic.co/search?package=nginx&prerelease=true&all=true" | jq -r '.[] | .version'
1.1.0
1.2.0
1.2.1
1.3.1
1.4.1
1.5.0
$ curl -s "https://epr.elastic.co/search?package=nginx&prerelease=true&experimental=true&all=true" | jq -r '.[] | .version'
0.2.3
0.2.4
0.3.6
0.3.7
0.3.9
0.4.0
0.5.0
0.7.0
1.1.0
1.2.0
1.2.1
1.3.1
1.4.1
1.5.0There was a problem hiding this comment.
Yes, this is what I mention in the FIXMEs, and what would be fixed by the changes in #891 around https://github.com/elastic/package-registry/pull/891/files#diff-20fd7b81f7ff43064db94851e0fc5ad6a9852949b07d72a02d48751be912534fL315
The problem is not with 0.x.y, versions, it is that these 0.x.y packages had release: experimental, and they are discarded by the condition checking experimental packages.
During the changes around the prerelease label (elastic/package-spec#225), there was also a push to move packages that we want to maintain out of experimental. This is why you can find multiple examples of 0.x packages that were experimental, updated directly to non-experimental 1.x versions. Most of the packages that only have versions with release: experimental are packages that we don't want to maintain on its current form, or have been replaced by other packages.
There was also some back and forth about how prerelease flag should behave on regard to the release flag (and how experimental should behave with semantic versioning), I don't remember the details, but we ended up with the implementation in #785.
Now, looking to the unit tests, there are some cases like the ones you mention that look wrong. But I am on the fence about "fixing" this, because this is also an opportunity to review packages that still use release: experimental. If they are to be maintained, new versions should be released following current approach, otherwise they won't be available after the change in Kibana (elastic/kibana#122973) is done.
In this PR I only wanted to add the unit tests, to make more clear what would change if we apply more changes of #891 in other PRs.
There was a problem hiding this comment.
Thanks for all the info! It puts everything into context 👍
And sure, adding those tests for sure would be beneficial
To test the changes proposed for https://github.com/elastic/ingest-dev/issues/1285, I started by moving the compatibility code in the handlers to the packages filter, so I can unit test the changes in the filter.
This is part of #891, that is still under discussion, but I think that we may want these unit tests in any case. They also help to illustrate other changes on this PR.