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.

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

Xpack Foo module on mage build system #9242

Merged
merged 9 commits into from
Nov 27, 2018

Conversation

sayden
Copy link
Contributor

@sayden sayden commented Nov 26, 2018

A merge of the commits that @andrewkroh mention here #9230 with the contents of a dummy foo module.

@ruflin take a look here too, I prefer to do a stop & go, ask and ensure the next step :)

andrewkroh and others added 4 commits November 26, 2018 19:10
This enables the use of mage to build, test, and package x-pack/metricbeat. This adds it to
the test matrix on Travis CI as well.

But it does not modify the top-level metricbeat build to stop producing x-pack artifacts. This
cut-over needs still needs to be done.

(cherry picked from commit d6b1ba1)

# Conflicts:
#	metricbeat/Dockerfile
return mage.PythonNoseTest(mage.DefaultPythonTestUnitArgs())
}

// PythonUnitTest executes the python system tests in the integration environment (Docker).

Choose a reason for hiding this comment

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

comment on exported function PythonIntegTest should be of the form "PythonIntegTest ..."

Copy link
Member

Choose a reason for hiding this comment

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

My bad. Can you fix this @sayden?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure! :)

@sayden sayden added review Metricbeat Metricbeat Team:Integrations Label for the Integrations team labels Nov 26, 2018
if err != nil {
return err
}
//defer os.Remove(modulesConfigYml)
Copy link
Member

@andrewkroh andrewkroh Nov 26, 2018

Choose a reason for hiding this comment

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

This should have been uncommented. We want to remove the file when this function finishes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok!

@andrewkroh
Copy link
Member

andrewkroh commented Nov 26, 2018

Looks like the foo module needs formatted.

check: Checking for modified files or incorrect permissions
Error: some files are not up-to-date. Run 'mage fmt update' then review and commit the changes. Modified: [x-pack/metricbeat/module/foo/bar/bar.go x-pack/metricbeat/module/foo/doc.go]
make[1]: *** [check] Error 1

@andrewkroh
Copy link
Member

andrewkroh commented Nov 26, 2018

When running mage integTest it expects to find a docker-compose.yml file. You can copy the one added in the mssql PR, but remove the mssql container defined in that file.

https://travis-ci.org/elastic/beats/jobs/459953763#L599

@ruflin
Copy link
Member

ruflin commented Nov 27, 2018

I checked out the branch locally and the following files are not checked in:

        include/
        metricbeat.reference.yml
        metricbeat.yml
        modules.d/

I think all these should be checked in too?

@andrewkroh I'm a bit surprised that we did not get an error on CI because of the above files.

@andrewkroh
Copy link
Member

andrewkroh commented Nov 27, 2018

The implementation of mage.Check uses very similar commands as our existing top-level Makefile. Neither will alarm on untracked files from what I understand, probably because this would have been to noisy for local usage. But once they are added make/mage check would alert if they were modified in any way.

beats/Makefile

Lines 88 to 89 in d997a82

@git update-index --refresh
@git diff-index --exit-code HEAD --

out, err := sh.Output("git", "diff-index", "HEAD", "--", ".")

But I do think it would be nice to have it fail on this condition in CI.

@ruflin
Copy link
Member

ruflin commented Nov 27, 2018

Got it. We can add such a check in a more generic way later. Thanks

@sayden Can you add the missing files?

Copy link
Member

@ruflin ruflin left a comment

Choose a reason for hiding this comment

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

LGTM.

@sayden sayden merged commit 37b67b0 into elastic:master Nov 27, 2018
@ruflin
Copy link
Member

ruflin commented Nov 27, 2018

@kaiyan-sheng Merged. You should rebase your PR on top of this.

@sayden Do you feel comfortable to follow up with a PR for the docs collection?

In general I think we should backport this as soon as we have at least one module in x-pack for 6.x. Also as soon as we have one module in, we should remove the "fake module".

ruflin added a commit to ruflin/beats that referenced this pull request Nov 27, 2018
ruflin added a commit that referenced this pull request Nov 27, 2018
@@ -1,21 +1,16 @@
FROM golang:1.11.2
MAINTAINER Nicolas Ruflin <ruflin@elastic.co>
FROM golang:1.10.3
Copy link
Member

Choose a reason for hiding this comment

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

@sayden Thanks to #11330 we found this change. I wonder if any of the changes here were intended / needed?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

As far as I remember this we were using 1.10.3 because of needs in the CI (formatting if I remember well) and probably. I don't remember to directly change this, I think it was a co-authored PR with @andrewkroh who maybe can give us some more light here.

Copy link
Member

Choose a reason for hiding this comment

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

It might be that things changed during the initial opening of the PR and when it get merged. Could you try a PR to get back the "old" Dockerfile again or have a look on what the actual differences and implications could be?

@sayden sayden deleted the xpack-foo-module-on-mage-build-system branch July 30, 2022 05:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Metricbeat Metricbeat review Team:Integrations Label for the Integrations team
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants