-
Notifications
You must be signed in to change notification settings - Fork 18
Expand mage and add build options #73
Conversation
|
/test |
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.
Added a few comments, but @narph this looks really good thanks for pushing theses changes!
| goTestJUnit(options: '-v ./...', output: 'junit-report.xml') | ||
| withMageEnv(){ | ||
| cmd(label: 'Go unitTest', script: 'mage unitTest') | ||
| } |
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.
Not familiar with the withMageEnv call but I assume it generates the junit-report.xml ?
| - amd64 | ||
| - arm64 | ||
| flags: | ||
| - -buildmode=pie |
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.
I believe we have more flags than that? like -s?
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.
Question: Does goreleaser support creating universal binary or we have to have a post build hook to merge both?
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.
we might be lucky using this https://goreleaser.com/customization/universalbinaries/
| - '{{ if eq .Env.DEV "true" }}all=-N -l{{ end }}' | ||
| changelog: | ||
| skip: true | ||
| dist: build/binaries No newline at end of file |
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.
This would be simple to share with different projects.
| func GoUnitTest(ctx context.Context) error { | ||
| mg.Deps(InstallGoTestTools) | ||
|
|
||
| fmt.Println(">> go test:", "Unit Testing") //nolint:forbidigo // just for tests |
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.
I think if you are using a Print* methods and a writer, it should not mark any usage.
out := os.stdout
fmt.Fprintln(out, >> go test:", "Unit Testing) The above allow you to enable the output or not.
|
Running |
|
|
|
Afaik we don't need to support 32bits https://www.elastic.co/support/matrix |
@cmacknz , the build has cgo disabled (implicitly enabling static linking) and uses Go's native cross compilation which simplifies the builds. |
|
|
||
|
|
||
| # VSCode | ||
| /.vscode |
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.
why viscose disappeared?
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.
it's still there, in #directories
The build is still failing for me, let me know if you need me to investigate anything. |
| if val := os.Getenv(buildEnv); val != "" { | ||
| boolVal, err := strconv.ParseBool(val) | ||
| if err != nil { | ||
| return defaultValue |
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.
can we shoot a warning here? maybe you want to know you provided incorrect value, may be a typo
| ) | ||
| } | ||
|
|
||
| testArgs = append(testArgs, []string{"./..."}...) |
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.
is windows ok with /?
| var ( | ||
| goLicenserRepo = "github.com/elastic/[email protected]" | ||
| const ( | ||
| GoreleaserRepo = "github.com/goreleaser/[email protected]" |
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.
please ignore this if you don't like it, but I would separate .1.6.3 to a variable ending with Version so in case I forgot where it is defined I can guess based on full text search
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.
few comments along the way, but overall it looks good
| } | ||
| if isSnapshot { | ||
| args = append(args, "--snapshot") | ||
| env["DEFAULT_VERSION"] += fmt.Sprintf("-%s", "SNAPSHOT") |
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.
[Suggestion]
the fmt.Sprintf is unnecessary
| env["DEFAULT_VERSION"] += fmt.Sprintf("-%s", "SNAPSHOT") | |
| env["DEFAULT_VERSION"] += "-SNAPSHOT" |
| return devtools.GoUnitTest(ctx, testCoverage) | ||
| } | ||
|
|
||
| //CHECKS |
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.
[NIT / Consistency]
| //CHECKS | |
| // CHECKS |
magefile.go
Outdated
| } | ||
|
|
||
| // CheckLicense checks the license headers | ||
| // License should generate the license headers |
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.
[Suggestion]
Personally, I find better to have a function description to be assertive, it does either A or B, not it "should" do something.
| // License should generate the license headers | |
| // License generates the license headers or returns an error |
I ran locally with a recent version of goreleaser which contains |
|
Build is fixed for me, thanks! |
Implement build solution using goreleaser (https://goreleaser.com/intro/)
.goreleaser.yamlbuild optionsExpand .CI
Expand mage and add build options
mageto list targetsEx:
mage build:all=ENV PLATFORMS=all mage buildbuilds all binariesmage buildbuilds binary for host machineenv PLATFORMS=windows mage buildto build all binaries for windowsmage unitTestto run all unit tests