-
Notifications
You must be signed in to change notification settings - Fork 17
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!
withGoEnv(){ | ||
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 |
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/go-licenser@v0.4.1" | ||
const ( | ||
GoreleaserRepo = "github.com/goreleaser/goreleaser@v1.6.3" |
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.yaml
build optionsExpand .CI
Expand mage and add build options
mage
to list targetsEx:
mage build:all
=ENV PLATFORMS=all mage build
builds all binariesmage build
builds binary for host machineenv PLATFORMS=windows mage build
to build all binaries for windowsmage unitTest
to run all unit tests