Skip to content
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

Remove support for Elasticsearch requirement #516

Merged
merged 4 commits into from
Jun 17, 2020

Conversation

ruflin
Copy link
Member

@ruflin ruflin commented Jun 17, 2020

So far I have not seen a package that would need to specify requirements for an Elasticsearch version as the Elasticsearch and Kibana version are tied together in the minor. Because of this removing support for Elasticsearch requirements for now. We can readd it later if we need.

This also cleans up the existing packages to remove the mention of Elasticsearch and some other examples that were still around.

As a follow up, integrations packages should be updated to get rid of the requirement. But more changes will happen to the requirements after #515 is merged so perhaps we rather wait with this a bit more.

This change should not break any existing packages.

@ruflin ruflin requested a review from mtojek June 17, 2020 09:33
@ruflin ruflin self-assigned this Jun 17, 2020
@ruflin
Copy link
Member Author

ruflin commented Jun 17, 2020

@jonathan-buttner This will probably also need an update to the Endpoint package.

@elasticmachine
Copy link

elasticmachine commented Jun 17, 2020

💚 Build Succeeded

Pipeline View Test View Changes Artifacts preview

Expand to view the summary

Build stats

  • Build Cause: [Pull request #516 updated]

  • Start Time: 2020-06-17T18:20:43.740+0000

  • Duration: 9 min 1 sec

Test stats 🧪

Test Results
Failed 0
Passed 105
Skipped 0
Total 105

@mtojek
Copy link
Contributor

mtojek commented Jun 17, 2020

I think you should rollout this the other way round. First, make sure nobody uses/publishes the field, then remove it.

What about existing packages? I think @jonathan-buttner raised a similar issue a day ago (backwards incompatible change).

@ruflin
Copy link
Member Author

ruflin commented Jun 17, 2020

As the field has not effects I would consider this "nobody is using it" ;-) As it doesn't break any existing packages, I think it is fine to get this in first.

For the existing packages already released, I would not touch them as additional data can be there but will not be used.

@mtojek
Copy link
Contributor

mtojek commented Jun 17, 2020

As the field has not effects I would consider this "nobody is using it" ;-) As it doesn't break any existing packages, I think it is fine to get this in first.

Usage:
https://github.com/elastic/integrations/blob/master/dev/import-beats/requirement.go#L30

For the existing packages already released, I would not touch them as additional data can be there but will not be used.

I agree with this approach :)

@ruflin
Copy link
Member Author

ruflin commented Jun 17, 2020

@mtojek Will you take care of the integrations usage or should I open a PR there?

So far I have not seen a package that would need to specify requirements for an Elasticsearch version as the Elasticsearch and Kibana version are tied together in the minor. Because of this removing support for Elasticsearch requirements for now. We can readd it later if we need.

This also cleans up the existing packages to remove the mention of Elasticsearch and some other examples that were still around.

As a follow up, integrations packages should be updated to get rid of the requirement. But more changes will happen to the requirements after elastic#515 is merged so perhaps we rather wait with this a bit more.

This change should not break any existing packages.
@mtojek
Copy link
Contributor

mtojek commented Jun 17, 2020

@mtojek Will you take care of the integrations usage or should I open a PR there?

Sure, I can help here.

@ruflin
Copy link
Member Author

ruflin commented Jun 17, 2020

@mtojek Thanks

@jonathan-buttner
Copy link
Contributor

@jonathan-buttner This will probably also need an update to the Endpoint package.

Thanks! I'll make sure our repo is updated 👍

@ruflin
Copy link
Member Author

ruflin commented Jun 17, 2020

@mtojek as elastic/integrations#92 is merged, ok to get this one in?

@ruflin ruflin merged commit 04472b9 into elastic:master Jun 17, 2020
@ruflin ruflin deleted the remove-es-constraints branch June 17, 2020 18:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants