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

Improve build config issue error scenario #483 #487

Merged
merged 3 commits into from
Oct 27, 2021

Conversation

HeavyWombat
Copy link
Contributor

@HeavyWombat HeavyWombat commented Oct 25, 2021

Fixes #483

Introduced a log statement to see which build config is picked for a respective import path. Also, added piece of code to initialize the ID of a build config with a default value (index) so that the log statement always has an ID to show.

Added a check to verify that the build config path (set by dir and main) actually points to a local file. This should help with use cases as described in #483 to find out configuration issues more quickly.

Fixed the wording in the build config section of the README since it was misleading as towards import paths could be used in the main field. Removed obsolete netgo build tag from the example and fixed the -trimpath in the example, which had two instead of just one dash.

@google-cla google-cla bot added the cla: yes label Oct 25, 2021
@codecov-commenter
Copy link

codecov-commenter commented Oct 25, 2021

Codecov Report

Merging #487 (3a52ca2) into main (6447264) will decrease coverage by 0.17%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #487      +/-   ##
==========================================
- Coverage   53.13%   52.96%   -0.18%     
==========================================
  Files          36       36              
  Lines        1848     1854       +6     
==========================================
  Hits          982      982              
- Misses        715      718       +3     
- Partials      151      154       +3     
Impacted Files Coverage Δ
pkg/build/gobuild.go 58.18% <0.00%> (-0.26%) ⬇️
pkg/commands/config.go 55.55% <0.00%> (-1.97%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 6447264...3a52ca2. Read the comment docs.

@HeavyWombat HeavyWombat changed the title fix/issue 483 Improve build config issue error scenario #483 Oct 25, 2021
README.md Outdated
@@ -127,7 +127,7 @@ baseImageOverrides:
### Overriding Go build settings

By default, `ko` builds the binary with no additional build flags other than
`--trimpath` (depending on the Go version). You can replace the default build
`-trimpath` (depending on the Go version). You can replace the default build
Copy link
Member

Choose a reason for hiding this comment

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

While we're here, we removed the 1.13 compat code a while back, so we always pass -trimpath now, regardless of go version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Thanks.

README.md Outdated
@@ -139,8 +139,7 @@ builds:
env:
- GOPRIVATE=git.internal.example.com,source.developers.google.com
flags:
- -tags
- netgo
- -trimpath
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure we need to change this example. Can we revert this?

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. No problem. I was recently made aware that the netgo build flag is not necessary anymore, at least there where I was using it. That was why I thought we could remove it here as well. However, it serves as an example and therefore helps with regards to understanding the idea behind that field.

imjasonh
imjasonh previously approved these changes Oct 26, 2021
There is currently no indication whether `ko` picks one of the configured
build configurations from the `.ko.yaml` configuration file for a build.

Add log statement to print the build config being picked for the build.

Introduce default entry for build config `ID` in case it is not specified.
Add `os.Stat` to verify that the path that is configured in the build
configuration entry is valid. As a side effect, this will print out an error
message in case someone sets an import path like `github.com/google/ko` in
the `main` field of the build config.
@HeavyWombat
Copy link
Contributor Author

Had to rebase due to recent changes.

Fixed wrong command line flag `--trimpath` to `-trimpath`.
@imjasonh imjasonh merged commit b9f9268 into ko-build:main Oct 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Adding "main:" to .ko.yaml causes build to succeed with missing environment variables
3 participants