-
Notifications
You must be signed in to change notification settings - Fork 386
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
Print variant from builder #248
Conversation
jonjohnsonjr
commented
Nov 17, 2020
•
edited
edited
pkg/build/gobuild.go
Outdated
@@ -272,7 +272,11 @@ func build(ctx context.Context, ip string, platform v1.Platform, disableOptimiza | |||
cmd.Stderr = &output | |||
cmd.Stdout = &output | |||
|
|||
log.Printf("Building %s for %s/%s", ip, platform.OS, platform.Architecture) | |||
plat := path.Join(platform.OS, platform.Architecture) |
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.
Do you really want to couple the separator in the label between OS and Architecture based on the path separator ? What happens if you run this on Windows ? And if you have tooling that relies on that log output to be stable (yes, such things happen). Also, it is really not related to a file path.
So I highly recommend using printf with a fixed "/" separator.
Or even just +
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.
path
is fixed as "/"
, filepath
is based on your current platform.
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.
ok, thanks for the heads up (didn't know that). Fine with me if you consider the architecture label to be an URL like 'path' (still like the more explicit form better)
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.
Sure, done.
c25dcae
to
fee8639
Compare
Codecov Report
@@ Coverage Diff @@
## master #248 +/- ##
==========================================
- Coverage 37.19% 37.11% -0.08%
==========================================
Files 33 33
Lines 1460 1463 +3
==========================================
Hits 543 543
- Misses 828 831 +3
Partials 89 89
Continue to review full report at Codecov.
|