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

[Ingest Manager] Removal of requirements in packages breaks UI #70453

Closed
ruflin opened this issue Jul 1, 2020 · 6 comments
Closed

[Ingest Manager] Removal of requirements in packages breaks UI #70453

ruflin opened this issue Jul 1, 2020 · 6 comments
Assignees
Labels
Team:Fleet Team label for Observability Data Collection Fleet team

Comments

@ruflin
Copy link
Member

ruflin commented Jul 1, 2020

In elastic/package-registry#574 requirements were renamed to conditions. I did not expect this to break the UI as requirements could also be empty / missing. But it seems it breaks the UI when accessing an integrations details page.

Removing some code related to requirements solves the issue.

My suggestion for 7.9 is to remove the code and UI completely and we rethink it moving forward.

To reproduce it, you need to check out the registry from master and run it locally.

@ruflin ruflin added Team:Fleet Team label for Observability Data Collection Fleet team Ingest Management:beta1 labels Jul 1, 2020
@elasticmachine
Copy link
Contributor

Pinging @elastic/ingest-management (Team:Ingest Management)

@jfsiii
Copy link
Contributor

jfsiii commented Jul 1, 2020

I don't know for sure what broke in Kibana, but the old golang struct https://github.com/elastic/package-registry/pull/574/files#diff-3c2c8ec3d107f7be6f8119cde040fbc0L63 says there will always be a requirement key

So

  "requirement": {
    "kibana": {
      "versions": "\u003e=7.3.0 \u003c8.0.0"
    },
    "elasticsearch": {
      "versions": "\u003e7.0.1"
    }
  },

or

  "requirement": {
  },

but not missing entirely

@jfsiii
Copy link
Contributor

jfsiii commented Jul 1, 2020

To reproduce it, you need to check out the registry from master and run it locally.

This is change is live on https://epr.elastic.co/
Screen Shot 2020-07-01 at 2 39 27 PM

compare with experimental
Screen Shot 2020-07-01 at 2 39 20 PM

So it can be reproduced most places. Here's a failing dev cluster
Screen Shot 2020-07-01 at 2 36 56 PM

@ruflin
Copy link
Member Author

ruflin commented Jul 1, 2020

The fix for it is here.

The part I missed when removing requirements is that it seems it was a requirement in Kibana to even have an empty field. If we reintroduce it as constraints, we should make sure it is optional.

@jfsiii
Copy link
Contributor

jfsiii commented Jul 2, 2020

The part I missed when removing requirements is that it seems it was a requirement in Kibana to even have an empty field.

I don't understand what you mean by empty field. Can you say more or give a code sample?

I'm new to golang but the TS types

requirement: RequirementsByServiceName;
export type RequirementsByServiceName = Record<ServiceName, ServiceRequirements>;

match my understanding of the struct https://github.com/elastic/package-registry/blob/33c9f9df5881a879ced083cc626ba973f928b32d/util/package.go#L63

and the example data
Screen Shot 2020-07-01 at 7 52 33 PM

This seems like a breaking change on the registry response which means it impacts Kibana, but perhaps I'm misreading the code.

Along those lines, I'll open a PR to update the OpenAPI spec

@ruflin
Copy link
Member Author

ruflin commented Jul 3, 2020

@jfsiii You are correct here. It was kind of a "bug" on the registry side that it was required, agree its a breaking change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Team:Fleet Team label for Observability Data Collection Fleet team
Projects
None yet
Development

No branches or pull requests

4 participants