Golang library for validation: folder structure validation #14
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@ruflin @mtojek This PR is a WIP but using it in its current state to validate all packages in the @elastic/integrations repo produces the following results. I suspect some of the following validation errors need fixes in the package while others need fixes in the spec. Could you please let me know which is which. Thank you! |
2 tasks
|
Let me refer to errors. I'm afraid all of them slipped through the review phase.
I think this is a valid file. Take a look at the
This is optional at the moment. You can use this file to render samples in docs. I'm not convinces whether it should be mandatory or not.
I'm allergic to changelogs, probably @ruflin would like to elaborate on them :) I suppose we will have to start using them, but I'm not sure if people get mad, when you ask them to fill them up. |
mtojek
reviewed
Aug 10, 2020
|
|
||
| // ValidateFromPath validates a package located at the given path against the | ||
| // appropriate specification and returns any errors. | ||
| func ValidateFromPath(packageRootPath string) error { |
There was a problem hiding this comment.
This is the method I was referring in different PR: ValidateFromPath. I would be cool not to load specs every time.
There was a problem hiding this comment.
Yep, see my reply on the other PR: #12 (comment). Let's continue the conversation there.
|
@mtojek For changelog: We need a way to tell users of the packages on what change between versions. I'm not tied at all to changelogs but we need a solution. |
|
My 2 cents: I like the proposed CHANGELOG yml format. It captures what we need, but in a structured fashion. So one can imagine generating human-readable changelog documentation from it, etc.
@mtojek could you elaborate on why this is? maybe there's a drawback that I'm not seeing currently. |
💚 Build Succeeded
Expand to view the summary
Build stats
|
|
I've created #17 so we can have a discussion dedicated to changelogs over there. I'll copy over relevant comments from here so they are readily available. |
|
I'm fine with taking the CHANGELOG discussion out the scope of this PR. I believe you can mark it as optional or temporarily remove its definition from spec. |
|
CI complains about missing comments: |
|
++ on skipping changelog and follow up. |
|
After making updates to the folder validation per review feedback, validating all existing packages in the @elastic/integrations repository with this PR produces the following results. As you can see most packages are now valid, which is nice! Let's go through the remaining errors reported above. I think we have to allow this for now. There is a discussion I have started (#22) that might impact this validation but for now, I think we add it. I will update this PR to do so but just wanted to call it out here first in case anyone has any objections. I checked and the So these errors look like a case of ingest pipelines being defined as JSON instead of YAML. What do we want to do here? I know Beats could accept either format — is this what we want for Packages too? or do we want to require that packages only define their ingest pipelines in YAML (in which case the above ingest pipelines will need to be converted to YAML)? Also, I assume Kibana is able to load ingest pipelines from either format into Elasticsearch? @ruflin? |
|
Kibana knows how to load multiple ingest pipelines. @skh @neptunian to make sure this is correct. For json vs yaml: I think we should support both. Yaml allows us when we build ingest pipelines from scratch to add comments etc, but users just exporting pipelines from Elasticsearch and copy / paste it to a package will likely use json. Kibana supports both formats. (@skh ) |
Merged
Yes. The ingest manager will install all pipelines in the
Yes, both formats are supported. |
|
Thanks @ruflin and @neptunian for weighing in. Based on your feedback, I have now updated this PR. And as a result all integrations packages are now passing validation on this PR! @mtojek @ruflin would you mind re-reviewing this PR please? Thanks! |
ruflin
approved these changes
Aug 17, 2020
3 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements validation of a package's folder structure. Structure of package files is not yet implemented and will be done in a follow up PR.
Related: #4.