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

Implement check and add headers in mage #13215

Merged
merged 10 commits into from
Sep 18, 2019

Conversation

jsoriano
Copy link
Member

@jsoriano jsoriano commented Aug 9, 2019

Add a framework to wrap go tooling in mage, taken from go-txtfile.

Using it, reimplements all uses of go-licenser in mage.
go-licenser is vendored.

Replaces #13211, see comments there for more context.

@jsoriano jsoriano requested a review from urso August 9, 2019 14:56
@jsoriano jsoriano requested review from a team as code owners August 9, 2019 14:56
@jsoriano jsoriano self-assigned this Aug 9, 2019
dev-tools/mage/install.go Show resolved Hide resolved
dev-tools/mage/install.go Show resolved Hide resolved
dev-tools/mage/gotool/get.go Outdated Show resolved Hide resolved
magefile.go Outdated Show resolved Hide resolved
@@ -0,0 +1,255 @@
// Licensed to Elasticsearch B.V. under one or more contributor
Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member Author

Choose a reason for hiding this comment

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

At the end I changed it so same flag can be specified multiple times, what was needed for go-licenser when multiple -excludes are needed.

@jsoriano jsoriano mentioned this pull request Aug 9, 2019
@jsoriano
Copy link
Member Author

jenkins, test this again please

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

Would you consider integrating this into the dev-tools/mage/target/common's Check() and Fmt() too?

And instead of adding AddLicenseHeaders and CheckLicenseHeaders to each project you could update all the projects to have a mage import like

import (
...

	// mage:import
	_ "github.com/elastic/beats/dev-tools/mage/target/common"
)

auditbeat/magefile.go Outdated Show resolved Hide resolved
auditbeat/magefile.go Outdated Show resolved Hide resolved
@jsoriano
Copy link
Member Author

I will continue with this after #13239

@jsoriano jsoriano added in progress Pull request is currently in progress. and removed review labels Aug 15, 2019
@jsoriano jsoriano added review and removed in progress Pull request is currently in progress. labels Sep 12, 2019
@jsoriano
Copy link
Member Author

@andrewkroh @urso this would be ready for another look

Copy link
Member

@andrewkroh andrewkroh left a comment

Choose a reason for hiding this comment

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

LGTM

@jsoriano jsoriano merged commit 4131241 into elastic:master Sep 18, 2019
@jsoriano jsoriano deleted the mage-check-headers branch September 18, 2019 16:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants