-
Notifications
You must be signed in to change notification settings - Fork 527
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
Generate fields for APM package #4432
Conversation
💚 Build Succeeded
Expand to view the summary
Build stats
Test stats 🧪
Steps errors
Expand to view the steps failures
|
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.
Overall LGTM on a first review. My main question is how this integrates with any update workflow - will it be part of running make update
?
Do you plan on addressing the TODOs before labeling this ready for review? (I believe fetching ECS info from github rather than expecting local installation would make sense).
Exactly, it should be part of the target that generates the big fields.yml file.
Thanks for validating, I wasn't sure (I copied the |
apmpackage/cmd/gen-package/main.go
Outdated
var packageVersion string | ||
|
||
func main() { | ||
flag.StringVar(&packageVersion, "packageVersion", "0.1.0", "Package version") |
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.
Maybe add a TODO to get the default version from cmd/version.go
(or move that into a package and make it an exported variable)?
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.
done, lmk if i got it right
A couple of comment which I forgot to add:
|
I think there is. Eg. when new fields are added is easy to see in the diff the table generated for the readme file and check it is rendered properly. I don't have a strong opinion thou.
I can add a TODO for that, but not sure that is worth the trouble, tbh. It takes millisecs and we need an internet connection to push the changes anyways. |
Same, so keep it for now and maybe we'll revisit it later.
When we're updating the package, sure. I run |
Codecov Report
@@ Coverage Diff @@
## master #4432 +/- ##
==========================================
- Coverage 76.11% 76.09% -0.03%
==========================================
Files 158 158
Lines 9782 9782
==========================================
- Hits 7446 7444 -2
- Misses 2336 2338 +2
|
Wrote some actual docs in 2411077. Ill hand that off to Brandon for polishing and completing. I guess he will move that somewhere else and link instead; but it would be great a review now to make sure they are accurate and understandable. |
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 with the additional x-pack fields. Thanks!
I haven't been over it with a fine-tooth comb, but it looks good and safe - so we can adjust over time as we test if needed.
Co-authored-by: Andrew Wilkins <[email protected]>
Co-authored-by: Andrew Wilkins <[email protected]>
… into generate-package-fields
Adds an APM package for Fleet, with a script to generate fields.yml files and docs # Conflicts: # go.mod # include/fields.go
@jalvz not sure this needs additional manual testing - please feel free to remove the labels in case you don't think it brings value. |
Loooked into how to test this and decided to add a |
As title. For more details, head to the diff in the
Readme.md
file.Motivation/summary
Checklist
I have considered changes for:
How to test these changes
make update
apmpackage/apm
Related issues
#4004