Check and generate multiple readme files in one package#282
Check and generate multiple readme files in one package#282kaiyan-sheng merged 23 commits intoelastic:masterfrom kaiyan-sheng:additional_readmes
Conversation
|
Pinging @elastic/integrations (Team:Integrations) |
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Trends 🧪 |
mtojek
left a comment
There was a problem hiding this comment.
Probably the command output should be adjusted:
Build the package
README.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/README.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
billing.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/billing.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
cloudtrail.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/cloudtrail.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
cloudwatch.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/cloudwatch.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
dynamodb.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/dynamodb.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
ebs.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/ebs.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
ec2.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/ec2.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
elb.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/elb.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
lambda.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/lambda.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
natgateway.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/natgateway.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
rds.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/rds.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
s3.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/s3.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
sns.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/sns.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
sqs.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/sqs.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
transitgateway.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/transitgateway.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
usage.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/usage.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
vpcflow.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/vpcflow.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
vpn.md file rendered: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/test/packages/aws/docs/vpn.md
Package built: /Users/marcin.tojek/go/src/github.com/elastic/elastic-package/build/integrations/aws/999.999.999
We don't have to notify the developer multiple times that the package has been built :)
…package into additional_readmes
|
@mtojek @ycombinator The current CI failure is caused by: I see in several test packages, we have |
Actually it is the other way round. The doc template can be used to generate the README file, but it's not a hard requirement. Please take a look at the log integration: https://github.com/elastic/integrations/tree/master/packages/log (it's exactly this use case). |
Thanks! OK it's fixed now I believe. This PR is ready for another review :) Thank you @mtojek and @ycombinator ! |
cmd/build.go
Outdated
| cmd.Printf("%s file rendered: %s\n", docs.ReadmeFile, target) | ||
|
|
||
| for _, target := range targets { | ||
| if target != "" { |
There was a problem hiding this comment.
What is the case that the target is empty?
There was a problem hiding this comment.
It will be empty when README file is static and can't be generated from the template file. This came from generateReadme(fileName, packageRoot) function.
There was a problem hiding this comment.
Ah I see! It might be cleaner to skip this file on the lower layer (in the docs.UpdateReadmes()), so "" (empty string) won't leak here.
cmd/lint.go
Outdated
| outdatedStr := "" | ||
| errStr := "" | ||
| for _, f := range readmeFiles { | ||
| if !f.UpToDate { |
There was a problem hiding this comment.
I'm trying to capture all outdated file names here to display in the error message. It's not pretty again. Is there a better way to do so?
There was a problem hiding this comment.
In the other comment I suggested to return an error if something bad happens.
You can check if docs.AreReadmesUpToDate() returned an error and any readmeFiles. If so, you can print the error and conditionally iterate over readmeFiles. No need to concatenate single error message. You can return a generic error and use cmd.Printf to iterate over problematic files. We used to mix cmd.Printf and errrors.Wrap for such cases, you can take a look at cmd/build.go and internal/builder/packages.go.
mtojek
left a comment
There was a problem hiding this comment.
I think we're close to the final form. Thanks for all adjustments!
cmd/lint.go
Outdated
| outdatedStr := "" | ||
| errStr := "" | ||
| for _, f := range readmeFiles { | ||
| if !f.UpToDate { |
There was a problem hiding this comment.
In the other comment I suggested to return an error if something bad happens.
You can check if docs.AreReadmesUpToDate() returned an error and any readmeFiles. If so, you can print the error and conditionally iterate over readmeFiles. No need to concatenate single error message. You can return a generic error and use cmd.Printf to iterate over problematic files. We used to mix cmd.Printf and errrors.Wrap for such cases, you can take a look at cmd/build.go and internal/builder/packages.go.
cmd/build.go
Outdated
| cmd.Printf("%s file rendered: %s\n", docs.ReadmeFile, target) | ||
|
|
||
| for _, target := range targets { | ||
| if target != "" { |
There was a problem hiding this comment.
Ah I see! It might be cleaner to skip this file on the lower layer (in the docs.UpdateReadmes()), so "" (empty string) won't leak here.
ycombinator
left a comment
There was a problem hiding this comment.
Pending the ongoing changes from @mtojek's review, this PR LGTM.
…package into additional_readmes
|
@mtojek |
mtojek
left a comment
There was a problem hiding this comment.
Just a couple of nit-picks and you're good to go :)
Regarding the issue reported by CI:
test case failed: one or more problems with fields found in documents: [0] unexpected pipeline error: field [json] not present as part of path [json]
... I fixed it in the integrations repo: elastic/integrations#791 . Most likely you need to sync it here.
mtojek
left a comment
There was a problem hiding this comment.
Well done! Ship it :) (just wait for the green status)
mtojek
left a comment
There was a problem hiding this comment.
@kaiyan-sheng could you please describe/justify the fix you applied to the PR to make it green? I'm curious what exactly was the root cause :)
| // Add package-level vars | ||
| pkgVars := kibana.Vars{} | ||
| input := pkg.PolicyTemplates[0].FindInputByType(streamInput) | ||
| input.Vars = append(input.Vars, pkg.Vars...) |
There was a problem hiding this comment.
@ycombinator could you please verify this fix? I'm not quite sure about implications.
There was a problem hiding this comment.
My understanding of this change is that all the package-level vars, defined in the package's manifest.yml file, are being copied into to each input's vars list.
From a system test definition perspective, this means that the top-level vars in the system test definition file could be used to override not just input vars but now also the package vars. So I think this change makes sense.
The only thing I would change is move this line inside the if input != nil check below.
There was a problem hiding this comment.
Done! Thanks @ycombinator for the review!!
| @@ -487,6 +487,7 @@ func createPackageDatastream( | |||
| // Add package-level vars | |||
| pkgVars := kibana.Vars{} | |||
| input := pkg.PolicyTemplates[0].FindInputByType(streamInput) | |||
There was a problem hiding this comment.
This is something we'll have to look to support multiple policy templates.
Thanks for the review! This new AWS package manifest.yml file added all AWS credential variables under |
With the new package-spec update with input groups, we start to allow multiple readme files
.mdunder_dev/build/docsanddocs. This PR is to add the check and generate multiple readme files when runningelastic-package buildandelastic-package lint.elastic-package lintwhenbilling.mdandcloudtrail.mdare missingelastic-package buildoutputelastic-package lintwhen there is no file missing