-
Notifications
You must be signed in to change notification settings - Fork 67
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
Add prerelease search parameter and deprecate experimental #785
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
🤖 GitHub commentsTo re-run your PR in the CI, just comment with:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The PR looks good to me, but I left a few points which may lead to a longer discussion. Let me know what do you think.
@@ -9,20 +9,24 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 | |||
### Breaking changes | |||
|
|||
* Ignore the `internal` parameter in packages and `/search` requests. [#765](https://github.com/elastic/package-registry/pull/765) | |||
* Packages with major version 0 or with prerelease labels are only returned by search requests when they include `prerelease=true` or `experimental=true`. [#785](https://github.com/elastic/package-registry/pull/785) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Out of curiosity, do you think we should perform an exercise and check how many packages are affected because of this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Packages with prerelease tags: 1, apm-server 8.0.0-rc1
Packages with major version 0: 48, they are all marked as beta or experimental, except a couple of them. We will have to think what to do about them. The couple of ga ones should be probably versioned as 1.x.
Btw, I have also found that kibana, logstash and microsoft packages are marked as experimental, but have 1.x versions, we will have to check this too.
I can delay the merge of this till we clarify the status of these packages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implementation LGTM, let's just clarify affected packages.
Thinking about the (not few) packages with version 0.x, I am wondering if we would need three modes of operations for search:
@mtojek @akshay-saraswat wdyt about this third mode? |
The third option might be hard to explain to users why some 0.x packages are considered GAs. It would be for keeping the rule as simple as possible BTW having packages marked as 0.2.0 or 0.6.0 and not maintaining them gives also a bad experience :) Users may consider them as unstable to use and discouraged to use the platform. Maybe @ruflin can see some other benefits/drawbacks of this change that will help us take the best decision. |
Well, they should still appear as non-GA somehow in the UIs, but yes, this mode would be a bit confusing, as prereleases would be available for some packages but not for others. I would also prefer to keep rules simple for users, so they can clearly decide if they want prerelease packages or not in their deployments. Probably the best is to keep pushing for GA versions of maintained packages, and keep this option simple. |
We had a similar discussion in the early days of the package registry. Initially only the GA packages were shown by default but we ended up with 3 packages so the change was made to show all the packages by default for the first iterations. Back then we had the discussion to have a toggle in the Fleet UI or a separate tab to show the prerelease too. It would be good to sync up on this with the Fleet team (@jen-huang @joshdover @mostlyjason ) before you roll out the change. A pure technical thought: Every query param is a filter. That means by default ALL packages should be returned and from there the user can filter down. This would still allow Fleet to have |
Yep, I created elastic/kibana#122973 for the changes in Fleet.
This is not exactly true with the current implementation. By default all non-experimental packages are returned, Looking it from a pure data perspective, it makes more sense to return everything by default and filter down with parameters, but from the kind of operational data exposed here, I think it makes sense to have defaults that return more safe or relevant results, and use parameters to extend them in particular use cases. |
I had exactly the same "challenge" when I implemented it the first time. I can see both approaches work. The reason I think of the "filter down" part is there might become a point where the search index is served by Elasticsearch. In this scenario it would mean "adding" param to the request means under the hood the query is likely adjusted to remove a filter. But as said before, both approaches will work, go with your preference. |
Added changes to default release levels after this thread: elastic/package-spec#256 (comment) |
512535e
to
c1c1ab4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, I assume you will take the decision when to push this PR :)
@jsoriano Can you please update the README to reflect these changes. It's missing |
Good one, I'll do it, thanks! |
Implements support for prerelease semver tags in the package registry, in the context of elastic/package-spec#225.
After this change, the package registry will return versions of packages in development only if
prerelease=true
orexperimental=true
parameters are included in the search requests. This shouldn't be breaking in current versions of Kibana because Kibana is currently always includingexperimental=true
. A follow-up task will be created in Kibana to remove the uses ofexperimental
, and add support for optionally enablingprerelease
(elastic/kibana#122973).Summarizing changes here:
prerelease
parameter is added to include versions of packages in development in search requests.experimental
parameter is deprecated.experimental
is set to true,prerelease
is also set to true.