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

Write input package spec #328

Merged
merged 27 commits into from
May 24, 2022
Merged

Write input package spec #328

merged 27 commits into from
May 24, 2022

Conversation

mtojek
Copy link
Contributor

@mtojek mtojek commented May 4, 2022

Follow-up on #325

This PR defines the spec for the input package. I spotted a few problems with sample packages and addressed them here too:

  1. Forbid already deprecated the "release" tag for input packages.
  2. Replace "agent/stream" with "agent/input". Apparently, that directory was also defined for integration.
  3. Fix changelog URL link.

Notes:

In the "input" spec I refer to the "integration" spec files to prevent redundancy of definitions. I decided not to extract common parts as there are only two specifications.

@mtojek mtojek changed the title Write input spec Write input package spec May 4, 2022
@elasticmachine
Copy link

elasticmachine commented May 4, 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-24T13:22:29.421+0000

  • Duration: 3 min 50 sec

🤖 GitHub comments

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

  • /test : Re-trigger the build.

@mtojek mtojek self-assigned this May 4, 2022
@mtojek mtojek requested review from ruflin, joshdover and P1llus May 4, 2022 12:55
@mtojek mtojek marked this pull request as ready for review May 4, 2022 12:56
@mtojek mtojek requested a review from a team as a code owner May 4, 2022 12:56
@mtojek mtojek mentioned this pull request May 4, 2022
1 task
@ruflin
Copy link
Contributor

ruflin commented May 5, 2022

@joshdover Lets make sure we have the Fleet PR open to support this in parallel to make sure it will work as expected.

@mtojek
Copy link
Contributor Author

mtojek commented May 5, 2022

@joshdover Lets make sure we have the Fleet PR open to support this in parallel to make sure it will work as expected.

Don't hurry up with it, there is plenty of work to be done on the elastic-package and Package Registry side (covered in the design doc).

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.

In the "input" spec I refer to the "integration" spec files to prevent redundancy of definitions. I decided not to extract common parts as there are only two specifications.

I would still consider adding a folder with shared/common definitions, even if we only have two specifications by now. But I would do it in a follow up so it is clearer what is being added here.

versions/1/input/changelog.spec.yml Outdated Show resolved Hide resolved
versions/1/input/manifest.spec.yml Show resolved Hide resolved
versions/1/input/spec.yml Outdated Show resolved Hide resolved
Copy link
Contributor Author

@mtojek mtojek left a comment

Choose a reason for hiding this comment

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

I would still consider adding a folder with shared/common definitions, even if we only have two specifications by now. But I would do it in a follow-up so it is clearer what is being added here.

I admit that I aligned the implementation with a rule of three, so when there is a new type I can extract a common part. If you find it clearer to do it now, I can focus on it in a follow-up.

versions/1/input/manifest.spec.yml Show resolved Hide resolved
versions/1/input/spec.yml Outdated Show resolved Hide resolved
versions/1/input/changelog.spec.yml Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member

jsoriano commented May 9, 2022

I would still consider adding a folder with shared/common definitions, even if we only have two specifications by now. But I would do it in a follow-up so it is clearer what is being added here.

I admit that I aligned the implementation with a rule of three, so when there is a new type I can extract a common part. If you find it clearer to do it now, I can focus on it in a follow-up.

Ok, not a strong opinion, let's apply the rule of three 🙂

jsoriano
jsoriano previously approved these changes May 9, 2022
@mtojek
Copy link
Contributor Author

mtojek commented May 16, 2022

@joshdover Friendly reminder about this PR :)

Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

I've taken a thorough look based on the spec changes, but I'll admit it's hard to know if this supports everything we need in the UI...without knowing what we want the UI to look like. @akshay-saraswat I think we need to produce some mocks on this before we can merge this.

I also agree with @ruflin that we should have a Kibana PR up in parallel before merging to validate the spec changes before committing to them. That will require scheduling capacity on the Fleet UI team for 8.4. I spoke to @jen-huang about this yesterday.


Would it make sense for input packages to define a list of allowed data types for the data stream? For instance, httpjson could be used for metrics, logs, or traces, but not synthetics. Do we want this to be user configurable? It wasn't clear in the definition doc.

icons:
$ref: "../integration/manifest.spec.yml#/definitions/icons"
screenshots:
$ref: "../integration/manifest.spec.yml#/definitions/screenshots"
Copy link
Contributor

Choose a reason for hiding this comment

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

What screenshots would we ship for an input package?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like rather leave an option to place any picture here, even for instruction purposes. If you consider this to be useless, I can drop the field.

@@ -25,12 +25,7 @@ spec:
name: agent
Copy link
Contributor

Choose a reason for hiding this comment

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

What's the difference between the agent directory in a data_stream vs. one in the overall package? Can we document this in the spec?

Copy link
Contributor Author

@mtojek mtojek May 20, 2022

Choose a reason for hiding this comment

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

Agent in spec is used by few integrations, which don't need a data stream, but they are still integrations:

https://github.com/elastic/integrations/blob/main/packages/fleet_server/agent/input/agent.yml.hbs
https://github.com/elastic/package-storage/blob/production/packages/apm/8.2.0/agent/input/template.yml.hbs

I was confused too, but didn't want to merge "agent" definitions into single one. Let me know what you think.

@mtojek
Copy link
Contributor Author

mtojek commented May 20, 2022

I've taken a thorough look based on the spec changes, but I'll admit it's hard to know if this supports everything we need in the UI...without knowing what we want the UI to look like. @akshay-saraswat I think we need to produce some mocks on this before we can merge this.

I don't have preference about this. Maybe it's because of the fact, that there will be a lot work around it to make it live :) Maybe we should release the input type as a beta to unblock other works?

@joshdover
Copy link
Contributor

Maybe we should release the input type as a beta to unblock other works?

That works for me. Can we enforce somehow that no input packages get published as GA before we've moved the spec out of beta?

@mtojek
Copy link
Contributor Author

mtojek commented May 23, 2022

I think that we should be able to implement that feature. I will give it a try.

@mtojek
Copy link
Contributor Author

mtojek commented May 24, 2022

I added release: beta to the input spec. Considering this, I guess that we don't have any blockers to merge beta-spec?

@mtojek mtojek requested review from jsoriano and joshdover May 24, 2022 13:20
Copy link
Contributor

@joshdover joshdover left a comment

Choose a reason for hiding this comment

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

LGTM for beta

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.

👍

@mtojek mtojek merged commit 946594b into elastic:main May 24, 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