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.

Sign up for GitHub

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

Jump to bottom

0.15.3 has breaking issues error resolving image references: build: go build: exit status 2: flag provided but not defined: -platform #1317

Closed
houshengbo opened this issue May 17, 2024 · 11 comments · Fixed by #1318
Closed

0.15.3 has breaking issues error resolving image references: build: go build: exit status 2: flag provided but not defined: -platform #1317

houshengbo opened this issue May 17, 2024 · 11 comments · Fixed by #1318

Comments

@houshengbo
Copy link

0.15.3 has breaking issues

https://github.com/knative/serving/actions/runs/9127229170/job/25097079749?pr=15219

2024/05/17 11:05:55 Building knative.dev/serving/cmd/webhook for linux/amd64
Error: error processing import paths in "config/core/999-cache.yaml": error resolving image references: build: go build: exit status 2: flag provided but not defined: -platform
usage: go build [-o output] [build flags] [packages]
Run 'go help build' for details.
@imjasonh
Copy link
Member

@nmittler possibly related to some of your recent go flag changes? Can you take a look?

@dprotaso
Copy link
Contributor

To repro use the env-var

export KO_FLAGS="--platform=linux/amd64"

@dprotaso
Copy link
Contributor

Yeah git bisect shows

2a4c12f410f678ee72c2dcd8036ceb1b9b113c10 is the first bad commit
commit 2a4c12f410f678ee72c2dcd8036ceb1b9b113c10
Author: Nathan Mittler <nmittler@aviatrix.com>
Date:   Wed May 15 13:16:12 2024 -0700

    Add global flags and ldflags

    Fixes #1304

 docs/configuration.md                         | 25 ++++++++++++++
 pkg/build/gobuild.go                          | 50 ++++++++++++++++++---------
 pkg/build/gobuild_test.go                     | 49 ++++++++++++++++++++++++++
 pkg/build/options.go                          | 16 +++++++++
 pkg/commands/options/build.go                 | 20 ++++++++---
 pkg/commands/options/build_test.go            | 23 +++++++++---
 pkg/commands/options/testdata/config/.ko.yaml |  5 +++
 pkg/commands/resolver.go                      |  2 ++
 8 files changed, 165 insertions(+), 25 deletions(-)
bisect found first bad commit

@nmittler
Copy link
Contributor

hmm ... possibly #1314? In pkg/commands/options/build.go I added v.GetStringSlice("flags"). Not sure if this is interacting with KO_FLAGS?

@nmittler
Copy link
Contributor

nmittler commented May 17, 2024 •
edited
Loading

Actually .. is KO_FLAGS even an environment variable that ko previously supported? Maybe a viper thing?

@dprotaso
Copy link
Contributor

dprotaso commented May 17, 2024 via email

@nmittler
Copy link
Contributor

Unfortunately, I'm definitely not a viper expert :). I'm guessing that <EnvPrefix>_FLAGS is a default variable exposed and used by viper, but I don't see it mentioned in the docs. Any links on how this works?

@dprotaso
Copy link
Contributor

Actually sorry I made a mistake - KO_FLAGS is not a ko thing. I guess in your commit you're now reading it for some reason.

@dprotaso
Copy link
Contributor

I guess when you added that GetStringSlice it prepends KO in front of the envvar - https://github.com/ko-build/ko/blame/bb99eccfe235e7b583c857bb1bafbf45f72178d1/pkg/commands/options/build.go#L112

@dprotaso
Copy link
Contributor

dprotaso commented May 17, 2024 •
edited
Loading

maybe you should rename the flag? eg. go_flags or something? Then it can be set using KO_GO_FLAGS likewise with the ldflags- since those are go build specific

@nmittler
Copy link
Contributor

@dprotaso yeah, that's was my thought as well. Will send a PR shortly.

nmittler added a commit to nmittler/ko that referenced this issue May 17, 2024
@nmittler
Refactor global values to be defaults
bce112b 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with viper's autogenerated `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation
- Adds an integration test for the `KO_FLAGS` issue.
- Removes a few missed references to `vendor` in scripts.

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 17, 2024
@nmittler
Refactor global values to be defaults
9416aab 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with viper's autogenerated `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation
- Adds an integration test for the `KO_FLAGS` issue.
- Removes a few missed references to `vendor` in scripts.

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 17, 2024
@nmittler
Refactor global values to be defaults
7b4a960 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with viper's autogenerated `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation
- Adds an integration test for the `KO_FLAGS` issue.
- Removes a few missed references to `vendor` in scripts.

Fixes ko-build#1317
@nmittler nmittler mentioned this issue May 17, 2024
Merged
nmittler added a commit to nmittler/ko that referenced this issue May 18, 2024
@nmittler
Refactor global values to be defaults
35e1087 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with viper's autogenerated `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation
- Adds an integration test for the `KO_FLAGS` issue.
- Removes a few missed references to `vendor` in scripts.

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 18, 2024
@nmittler
Refactor global values to be defaults
9500660 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with knative's `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation
- Removes a few missed references to `vendor` in scripts.

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 18, 2024
@nmittler
Rename the environment var used for flags
965073e 
This is a hack to avoid a collision with knative's `KO_FLAGS` environment variable. Ko's recent addition of a global `flags` value results in viper reading this variable.

This PR forces viper to rename the environment variable used for `flags` to be `KO_BUILD_FLAGS` instead.

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 18, 2024
@nmittler
Refactor global values to be defaults
c953c97 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with knative's `KO_FLAGS` environment variable (see ko-build#1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation

Fixes ko-build#1317
nmittler added a commit to nmittler/ko that referenced this issue May 18, 2024
@nmittler
Rename the environment var used for flags
15b9964 
This is a hack to avoid a collision with knative's `KO_FLAGS` environment variable. Ko's recent addition of a global `flags` value results in viper reading this variable.

This PR forces viper to rename the environment variable used for `flags` to be `KO_BUILD_FLAGS` instead.

Fixes ko-build#1317
imjasonh pushed a commit that referenced this issue May 21, 2024
@nmittler @imjasonh
Refactor global values to be defaults
7cb29ac 
There was recent work to add global values for `env`, `flags`, and `ldflags`. The global values would be merged with per-build values to generate the value used for the builds.

There are a couple issues with this:

- It's inconsistent with the existing code, which only has `default` values declared globally (there is no merging today).
- The name of the `flag` variable, caused a conflict with knative's `KO_FLAGS` environment variable (see #1317)

This PR does the following:

- Refactors the logic to use `defaultEnv`, `defaultFlags`, and `defaultLdflags`. This resolves both issues described above.
- Updates documentation

Fixes #1317
@nmittler nmittler mentioned this issue May 21, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

Refactor global values to be defaults nmittler/ko
4 participants
@dprotaso @imjasonh @houshengbo @nmittler