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 errors/warnings for unnecessary .ko.yaml settings #542

Closed
imjasonh opened this issue Dec 14, 2021 · 3 comments · Fixed by #618
Closed

Improve errors/warnings for unnecessary .ko.yaml settings #542

imjasonh opened this issue Dec 14, 2021 · 3 comments · Fixed by #618
Labels
good first issue Good for newcomers

Comments

@imjasonh
Copy link
Member

From #530 (comment)

A few points to note:

  • The -B (--base-import-paths) and --bare flags are not designed to be used together, and in this case, -B takes precedence: https://github.com/google/ko#naming-images

  • The main field in .ko.yaml is a directory, not a full Go import path.

  • If your .ko.yaml and go.mod are in the same directory as your main package, and if this directory is also your working directory, then you can leave out dir and main.

  • ko sets CGO_ENABLED=0 by default.

Each of these could be warnings or errors to help make .ko.yaml easier to use and less needlessly verbose.

@imjasonh imjasonh added the good first issue Good for newcomers label Dec 14, 2021
@imjasonh
Copy link
Member Author

Another one:

  • ko publish --local is confusing if KO_DOCKER_REPO is set to anything besides ko.local, and even then it'd be nice not to have both options. Maybe we should just deprecate --local, but even if we don't we should at least log a warning about it.

@imjasonh imjasonh added this to the 0.10 milestone Dec 15, 2021
@imjasonh
Copy link
Member Author

For all of these I think we should log a warning, and note that they might become failures in a future release.

Then, after we cut a release (v0.10), we can make them failures.

@imjasonh
Copy link
Member Author

Another:

  • ko build ./ --platform=all,linux/arm64 only builds for linux/arm64 and doesn't even log a warning. This should probably just fail if you have all and anything else.

halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
halvards added a commit to halvards/ko that referenced this issue Jan 25, 2022
This change adds a dependency on `go-logr/logr` and replaces all uses of
the `log` package from Go's standard library with a `logr`-based
implementation.

To ease the transition, and to keep the diff small, this change
introduces a package `github.com/google/ko/pkg/log` that should be
imported in place of `log` from the standard library.

The new package provides some of the same functions as `log`, e.g.,
`Printf()` and `Fatalf()`. The replacement functions takes an extra
`context.Context` argument. The `Context` is used to look up the
`logr.Logger` to use for logging.

Existing log statements have been updated to use the transition
functions, with `Context` plumbed through where required.

New code can use the new logging syntax, e.g.:

```go
log.L(ctx).Info("something interesting happened", "key", "value")
```

or

```go
log.L(ctx).Error(err, "uh oh", "what", "snafu")
```

This change alters the log output to `logr`'s structured output, with
named fields for the log message and level. As an example, a log line
that previously looked like this:

```
2022/01/25 19:44:18 Building github.com/google/ko/test for linux/amd64
```

Will look like as follows with this change:

```
2022/01/25 19:46:02 ko: "level"=1 "msg"="Building github.com/google/ko/test for linux/amd64"
```

Fixes: ko-build#560
Related: ko-build#542
@imjasonh imjasonh removed this from the 0.10 milestone Feb 16, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant