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.

Sign up for GitHub

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

Jump to bottom

Fix generated metricbeat so create-metricset works. #18020

Merged
merged 2 commits into from
Apr 28, 2020
Merged

Fix generated metricbeat so create-metricset works. #18020

merged 2 commits into from
Apr 28, 2020

Conversation

blakerouse
Copy link
Contributor

What does this PR do?

Fixes the make -C generator/_templates/metricbeat test so that make create-metricset works inside of the generated custom metricbeat.

Why is it important?

Both so create-metricset works on the generated beat and so the CI passes.

Checklist

  • My code follows the style guidelines of this project
  • I have commented my code, particularly in hard-to-understand areas
  • [ ] I have made corresponding changes to the documentation
  • [ ] I have made corresponding change to the default configuration files
  • [ ] I have added tests that prove my fix is effective or that my feature works
  • [ ] I have added an entry in CHANGELOG.next.asciidoc or CHANGELOG-developer.next.asciidoc.

@botelastic botelastic bot added the needs_team Indicates that the issue/PR needs a Team:* label label Apr 27, 2020
@ycombinator ycombinator self-requested a review April 27, 2020 18:33
ycombinator
ycombinator reviewed Apr 27, 2020
View reviewed changes
metricbeat/scripts/mage/target/metricset/metricset.go
// Required ENV variables:
// * MODULE: Name of the module
// * METRICSET: Name of the metricset
func CreateMetricset() error {
Copy link
Contributor

Choose a reason for hiding this comment

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

For consistency with other targets' functions, how about taking a context.Context here as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Being that it's not using it, I removed it. Should we have it there even if its not being used?

Copy link
Contributor

@ycombinator ycombinator Apr 27, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, I could go either way over here. I see that other targets have it and it's not being used in them as well.

Personally, I'd prefer to leave it out if it's not being used (so 👍 to what you've done here) but perhaps we should also do that everywhere else in a follow up PR.

ycombinator
ycombinator approved these changes Apr 27, 2020
View reviewed changes
Copy link
Contributor

@ycombinator ycombinator left a comment

Choose a reason for hiding this comment

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

Left a nit, otherwise LGTM. Thanks for fixing! <3

@blakerouse blakerouse added the Team:Platforms Label for the Integrations - Platforms team label Apr 27, 2020
@elasticmachine
Copy link
Collaborator

Pinging @elastic/integrations-platforms (Team:Platforms)

@blakerouse blakerouse added bug and removed needs_team Indicates that the issue/PR needs a Team:* label labels Apr 27, 2020
@ycombinator
Copy link
Contributor

ycombinator commented Apr 27, 2020 •
edited
Loading

@blakerouse CI failure looks related.

beatpath/testmetricbeat imports
	github.com/elastic/beats/v7/metricbeat/scripts/mage/target/metricset: module github.com/elastic/beats@latest found (v7.6.2+incompatible), but does not contain package github.com/elastic/beats/v7/metricbeat/scripts/mage/target/metricset
Error: error while running go mod vendor: running "go mod vendor" failed with exit code 1

@blakerouse
Copy link
Contributor Author

Yeah its related, but do not know why its saying it is not present. From the commit you can see that it is present.

@kvch Any ideas why that would be the case?

@ycombinator
Copy link
Contributor

@blakerouse I wonder if you need to split this PR up into two. First, one introducing the github.com/elastic/beats/v7/metricbeat/scripts/mage/target/metricset package. Once that's merged, then a second one that uses it.

@ChrsMark ChrsMark mentioned this pull request Apr 28, 2020
Merged
@blakerouse blakerouse merged commit 77f0d20 into elastic:master Apr 28, 2020
@blakerouse blakerouse deleted the fix-generator-test branch April 28, 2020 13:04
v1v added a commit to v1v/beats that referenced this pull request Apr 28, 2020
@v1v
Merge remote-tracking branch 'upstream/master' into feature/support-r…
e9ff669 
…unbld

* upstream/master:
  ci: comment PRs with the build status (elastic#17971)
  Add domain state metricset to kvm module (elastic#17673)
  [Agent] Allow CLI paths override (elastic#17781)
  Fix generated metricbeat so create-metricset works. (elastic#18020)
  LIBBEAT: Enhancement replace_string processor for replacing strings values of fields. (elastic#17342)
  Update stale references to _xpack to refer to _license instead (elastic#18030)
  Review dependency patterns collection in Jenkins (elastic#18004)
@blakerouse blakerouse mentioned this pull request Apr 28, 2020
Merged
2 tasks
blakerouse added a commit to blakerouse/beats that referenced this pull request Apr 28, 2020
@blakerouse
Fix generated metricbeat so create-metricset works. (elastic#18020)
d1a459b 
* Fix generated metricbeat so create-metricset works.

* Fix mage target add target to x-pack/metricbeat.

(cherry picked from commit 77f0d20)
blakerouse added a commit that referenced this pull request Apr 28, 2020
@blakerouse
Fix generated metricbeat so create-metricset works. (#18020) (#18063)
dfb87a7 
* Fix generated metricbeat so create-metricset works.

* Fix mage target add target to x-pack/metricbeat.

(cherry picked from commit 77f0d20)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ycombinator ycombinator ycombinator approved these changes

@andrewkroh andrewkroh andrewkroh approved these changes

Assignees
No one assigned
Labels
bug Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.8.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@blakerouse @elasticmachine @ycombinator @andrewkroh @andresrc