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

Support input packages #809

Merged
merged 20 commits into from
Jun 1, 2022
Merged

Support input packages #809

merged 20 commits into from
Jun 1, 2022

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented May 5, 2022

Meta-issue: elastic/package-spec#319

Blocker: elastic/package-spec#328

This PR modifies the Package Registry to support "input packages".

Changes:

  1. Use sql_input as test package.
  2. Add missing fields to "package index" API method.
  3. Filter by "type" with /search API method.
  4. Regenerate expected test results.

@elasticmachine
Copy link

elasticmachine commented May 5, 2022

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-05-25T11:59:06.268+0000

  • Duration: 5 min 12 sec

Test stats 🧪

Test Results
Failed 0
Passed 158
Skipped 0
Total 158

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

@mtojek mtojek marked this pull request as ready for review May 5, 2022 13:06
@mtojek mtojek requested review from a team and ruflin May 5, 2022 13:06
"version": "1.0.1",
"release": "ga",
"description": "Execute custom queries against an SQL database and store the results in Elasticsearch.",
"type": "input",
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What are old versions of Kibana do with packages that are not integrations?

Should we filter by type: integration when no type is passed? and maybe have a type: all to list all packages.

Or maybe this is a point where we should start versioning the API?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

and maybe have a type: all to list all packages.

I wouldn't like to design an API that won't be used by Fleet at all (KISS).

Should we filter by type: integration when no type is passed?

That would be counter-intuitive, you want to search for packages, but for some historical reason, the /search filters some of them. I would only go with that option to prevent any problems on the Fleet side.

Or maybe this is a point where we should start versioning the API?

It's always a good practice to version it, but not sure if this is a "dead end" to introduce versioning or if we just enable another GET parameter.

Copy link
Member

@jsoriano jsoriano May 10, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, then going back to the first question, what are current/old versions of Kibana going to do with packages that are not integrations?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Passing the question to @joshdover.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems like something that could easily be tested manually without code-level knowledge of Fleet. That would be how I test this personally.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea, but, to be honest, I wouldn't like to block this PR until that happens.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah yeah, I didn't want to mean that 🙂 But this may delay declaring input package as GA in the spec.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mtojek I would say that each new input package should start with a kibana version constraints higher or equals to 8.3 which should then do the trick isn't it?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, Julien, that's the idea. Just waiting for the confirmation/approval from @joshdover.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Kibana version constraint enforced by package-spec SGTM, agreed it can be done as a follow up.

main_test.go Outdated Show resolved Hide resolved
Copy link
Member

@jsoriano jsoriano left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, waiting for the discussion about backwards compatibility with Kibana/Fleet.

main_test.go Outdated Show resolved Hide resolved
@mtojek mtojek marked this pull request as draft May 25, 2022 10:01
@mtojek mtojek requested review from joshdover and jsoriano May 25, 2022 10:54
@mtojek mtojek marked this pull request as ready for review May 25, 2022 10:54
@mtojek
Copy link
Contributor Author

mtojek commented May 25, 2022

As the input-type spec has been released, I suppose that this is the next task to be reviewed/merged.

mtojek added 2 commits May 25, 2022 13:25
jlind23
jlind23 approved these changes May 25, 2022
Copy link
Contributor

@jlind23 jlind23 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@mtojek mtojek merged commit e7f04cf into elastic:main Jun 1, 2022
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.

5 participants