-
Notifications
You must be signed in to change notification settings - Fork 458
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
Adjust integrations to pass linting #266
Conversation
Pinging @elastic/integrations (Team:Integrations) |
@@ -11,7 +11,6 @@ categories: | |||
release: experimental | |||
conditions: | |||
kibana.version: ">=7.10.0" | |||
agent.version: ">=7.10.0" |
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.
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.
We don't have support for a required Agent version so I would remove it. But happy to have a conversation if this is needed.
@@ -31,7 +31,7 @@ streams: | |||
Success TTL for reverse DNS lookup on remote IP addresses in the socket metricset. | |||
|
|||
- name: socket.reverse_lookup.failure_ttl | |||
type: duration |
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.
@ruflin we don't support "duration" type, right?
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.
@jen-huang Can you comment on this? I know we played with the idea. Would it be useful to have it?
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.
It's rather question - whether it's supported at the moment :)
According to the reference package, it's not: https://github.com/elastic/package-registry/blob/33c9f9df5881a879ced083cc626ba973f928b32d/testdata/package/reference/1.0.0/dataset/reference/manifest.yml#L47
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.
Ok, I will merge the fix to prevent potential merge conflicts. In case of emergency I will open another PR.
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.
Right now the UI only differentiates between type yaml
and not-yaml. Not-yaml fields are treated as text, so right now we technically support "any" type. I think we had an initial list of other types for future UI improvements (special inputs, specific validation, etc), but we haven't gotten around to those improvements yet.
IMO it would be good to keep this duration
type around if we expect this sort of field to be fairly common in multiple packages. It won't break anything in the UI. Or maybe we should circle back and adjust the initial type list we made, now that packages are more fleshed out?
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.
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.
To make validation possible, I suggest we only use the ones supported and if we add a new one adjust the packages. Otherwise we suddenly have "magical" changes. No new packages was released but it looks now different.
@mtojek What are the types you are missing at the moment? Lets create a list with the priority.
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.
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 pending resolution to unresolved conversations with other reviewers.
What does this PR do?
This PR enables linting for integrations resources (file content).
Checklist
Author's Checklist
How to test this PR locally
Related issues
Screenshots