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

Adding "main:" to .ko.yaml causes build to succeed with missing environment variables #483

Closed
zikaeroh opened this issue Oct 20, 2021 · 6 comments · Fixed by #487
Closed

Comments

@zikaeroh
Copy link

This is a re-report from reddit.

I have this as my config:

defaultBaseImage: gcr.io/distroless/base:nonroot
builds:
  - id: main
    ldflags:
      - -X github.com/hortbot/hortbot/internal/version.version={{.Env.HB_VERSION}}

When I run ko, I get an error as expected:

$ ko publish --local --preserve-import-paths .
2021/10/19 18:36:14 Using base gcr.io/distroless/base:nonroot for github.com/hortbot/hortbot
Error: failed to publish images: error building "ko://github.com/hortbot/hortbot": template: argsTmpl:1:61: executing "argsTmpl" at <.Env.HB_VERSION>: map has no entry for key "HB_VERSION"
2021/10/19 18:36:14 error during command execution:failed to publish images: error building "ko://github.com/hortbot/hortbot": template: argsTmpl:1:61: executing "argsTmpl" at <.Env.HB_VERSION>: map has no entry for key "HB_VERSION"

But, change the config to this (adding main:):

defaultBaseImage: gcr.io/distroless/base:nonroot
builds:
  - id: main
    main: github.com/hortbot/hortbot
    ldflags:
      - -X github.com/hortbot/hortbot/internal/version.version={{.Env.HB_VERSION}}

And now it builds, ignoring the missing variable:

$ ko publish --local --preserve-import-paths .
2021/10/19 18:36:21 Using base gcr.io/distroless/base:nonroot for github.com/hortbot/hortbot
2021/10/19 18:36:22 Building github.com/hortbot/hortbot for linux/amd64
2021/10/19 18:36:24 Loading ko.local/github.com/hortbot/hortbot:c8b7f4f7d18e62e542dd1206b7aaa250cae4823dcd47de90ee2e1c9d0743ee55
2021/10/19 18:36:24 Loaded ko.local/github.com/hortbot/hortbot:c8b7f4f7d18e62e542dd1206b7aaa250cae4823dcd47de90ee2e1c9d0743ee55
2021/10/19 18:36:24 Adding tag latest
2021/10/19 18:36:24 Added tag latest
ko.local/github.com/hortbot/hortbot:c8b7f4f7d18e62e542dd1206b7aaa250cae4823dcd47de90ee2e1c9d0743ee55
$ docker run ko.local/github.com/hortbot/hortbot version                               
devel

Adding main: . doesn't hit this (presumably because it's the default).

Perhaps it's intended to be an error to specify a full path?

@imjasonh
Copy link
Member

cc @HeavyWombat in case he knows what might be causing this

@imjasonh
Copy link
Member

Perhaps it's intended to be an error to specify a full path?

I think that's the case, yeah. I've tested this with the ./test/ package in this repo, with main: ./test/ (see #480) and it works as expected, even with {{ .Env.FOO }} placeholders.

So I think main: . is the expected value in your case.

In the docs for goreleaser (whose config we're borrowing from here) it says:

    # Path to main.go file or main package.

So it sounds like this is supposed to be a file path, and not a full Go importpath. I wonder if we could detect the importpath case and fail with a better descriptive error message, instead of just silently proceeding. 🤔

@zikaeroh
Copy link
Author

zikaeroh commented Oct 20, 2021

Presumably, this could be a general check that just verifies that the specified directory or file actually exists; that would have caught my mistake. I don't think it has to be an import path specific case (since reasonably I could write test and that points to a directory named test).

Though, I don't exactly know how this led to the environment variable not working!

@HeavyWombat
Copy link
Contributor

Sorry for the late reply. I will have a look, too. If I am not mistaken, both the dir and the main field should have a default. For main, it could be that . should be the default. The sections in the README tried to explain that, but maybe this could be improved to be more clear.

@HeavyWombat
Copy link
Contributor

HeavyWombat commented Oct 25, 2021

Had a look now and I totally misunderstood the issue in the beginning. With main set to an import path, it "only" runs through like it does because it does not match the current build with any of the build configs and therefore builds with an empty default build config in which no environment variables are processed. Therefore, it just runs through.

My two key takeaways here are:

  • The error message could be better in case an environment variable is specified that is not set.
  • When a build configuration is used, there should be a check that dir plus main actually point to a local file. This would include improving the docs.

@zikaeroh
Copy link
Author

zikaeroh commented Oct 25, 2021

I'm not sure I follow on the first point. The error message is fine when it works (map has no entry for key "HB_VERSION"), it's just that when I misconfigured main, that check was seemingly never performed, so I didn't know that I hadn't properly set HB_VERSION until I had already published and went to use my image.

imjasonh pushed a commit that referenced this issue Oct 27, 2021
* Add build config usage log statement

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 path check for build configuration settings

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.

* Fix trimpath command line flag in README

Fixed wrong command line flag `--trimpath` to `-trimpath`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants