Skip to content

Fix generated metricbeat so create-metricset works.#18020

Merged
blakerouse merged 2 commits intoelastic:masterfrom
blakerouse:fix-generator-test
Apr 28, 2020
Merged

Fix generated metricbeat so create-metricset works.#18020
blakerouse merged 2 commits intoelastic:masterfrom
blakerouse:fix-generator-test

Conversation

@blakerouse
Copy link
Copy Markdown
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
// Required ENV variables:
// * MODULE: Name of the module
// * METRICSET: Name of the metricset
func CreateMetricset() error {
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
Contributor

@ycombinator ycombinator Apr 27, 2020

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.

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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
Copy Markdown
Contributor

ycombinator commented Apr 27, 2020

@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
Copy Markdown
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
Copy Markdown
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.

@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
…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 added a commit to blakerouse/beats that referenced this pull request Apr 28, 2020
* 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
* 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

Labels

bug Team:Integrations Label for the Integrations team Team:Platforms Label for the Integrations - Platforms team v7.8.0

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants