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

Add flag and PublishOption for destination repo #351

Merged
merged 4 commits into from
Apr 30, 2021

Conversation

halvards
Copy link
Collaborator

This enables programmatically setting the destination image repository
when embedding ko's publish functionality in other tools.

See discussion in #348 for additional context.

This enables programmatically setting the destination image repository
when embedding ko's `publish` functionality in other tools.

See ko-build#348
@google-cla google-cla bot added the cla: yes label Apr 29, 2021
halvards added a commit to halvards/ko that referenced this pull request Apr 29, 2021
This commit removes the `DockerRepo` config option and `--docker-repo`
flag from the PR.

New PR with the extracted config option:
ko-build#351
@codecov-commenter
Copy link

codecov-commenter commented Apr 29, 2021

Codecov Report

Merging #351 (00383f7) into main (d498734) will increase coverage by 0.03%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #351      +/-   ##
==========================================
+ Coverage   38.24%   38.27%   +0.03%     
==========================================
  Files          33       33              
  Lines        1527     1523       -4     
==========================================
- Hits          584      583       -1     
+ Misses        849      846       -3     
  Partials       94       94              
Impacted Files Coverage Δ
pkg/commands/resolver.go 11.66% <0.00%> (ø)
pkg/publish/daemon.go 29.41% <0.00%> (-2.59%) ⬇️
pkg/commands/run.go 0.00% <0.00%> (ø)
pkg/commands/apply.go 0.00% <0.00%> (ø)
pkg/commands/create.go 0.00% <0.00%> (ø)
pkg/commands/delete.go 0.00% <0.00%> (ø)
pkg/commands/publish.go 0.00% <0.00%> (ø)
pkg/commands/resolve.go 0.00% <0.00%> (ø)
pkg/publish/kind/write.go 82.85% <0.00%> (ø)

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 d498734...00383f7. Read the comment docs.

@imjasonh
Copy link
Member

Thanks for making this change. I think I'm convinced that grabbing the env var doesn't belong in makePublisher, but instead in an option like you're adding with this PR. 👍

I'm still a bit less convinced that it therefore needs to be a separate flag. Can we compromise and make AddPublishArg grab the env var and set the option for now?

Then, to embed this in skaffold (or elsewhere) we can make makePublisher and any other method you need exported so you can call them from your code. That way you can still embed ko logic, and ko doesn't have to add a flag (...yet).

wdyt?

@halvards
Copy link
Collaborator Author

Thanks for the quick reply! Yes, that SGTM, I'll go ahead and make the change.

The flag isn't required for embedding, I just added it for consistency, since all the other fields in PublishOptions are exposed as flags.

This enables programmatically setting the destination image repository
and avoids exposing a flag.
Copy link
Member

@imjasonh imjasonh left a comment

Choose a reason for hiding this comment

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

Lookin' good, just a few small things.

README.md Outdated
@@ -68,6 +68,9 @@ e.g.:
- `KO_DOCKER_REPO=gcr.io/my-project`, or
- `KO_DOCKER_REPO=my-dockerhub-user`

If both the flag and the environment variable are set, the flag value takes
Copy link
Member

Choose a reason for hiding this comment

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

This change can be removed now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Done

pkg/commands/options/publish.go Outdated Show resolved Hide resolved
pkg/commands/options/publish.go Outdated Show resolved Hide resolved
@imjasonh imjasonh merged commit 516cdee into ko-build:main Apr 30, 2021
halvards added a commit to halvards/ko that referenced this pull request May 24, 2021
- Export functions and a variable to enable embedding of ko's
  `publish` functionality to be embedded in other tools.

  See GoogleContainerTools/skaffold#5611

- Remove DockerRepo PublishOption and flag.

  This removes the `DockerRepo` config option and `--docker-repo`
  flag from the PR.

  New PR with the extracted config option:
  ko-build#351

- Fix copyright headers for boilerplate check.

- Use DockerRepo PublishOption instead of env var.

- Override defaultBaseImage using BuildOptions.

  Remove exported package global SetDefaultBaseImage and instead
  allow programmatic override of the default base image using
  the field `BaseImage` in `options.BuildOptions`.

  Also fix copyright header years.

- Add BuildOptions parameter to getBaseImage

  This enables access to BaseImage for programmatically overriding
  the default base image from `.ko.yaml`.

- Add UserAgent to BuildOptions and PublishOptions

  This enables programmatically overriding the `User-Agent` HTTP
  request header for both pulling the base image and pushing the
  built image.

- Rename MakeBuilder to NewBuilder and MakePublisher to NewPublisher.

  For more idiomatic constructor function names.
halvards added a commit to halvards/ko that referenced this pull request May 24, 2021
- Export functions and a variable to enable embedding of ko's
  `publish` functionality to be embedded in other tools.

  See GoogleContainerTools/skaffold#5611

- Remove DockerRepo PublishOption and flag.

  This removes the `DockerRepo` config option and `--docker-repo`
  flag from the PR.

  New PR with the extracted config option:
  ko-build#351

- Fix copyright headers for boilerplate check.

- Use DockerRepo PublishOption instead of env var.

- Override defaultBaseImage using BuildOptions.

  Remove exported package global SetDefaultBaseImage and instead
  allow programmatic override of the default base image using
  the field `BaseImage` in `options.BuildOptions`.

  Also fix copyright header years.

- Add BuildOptions parameter to getBaseImage

  This enables access to BaseImage for programmatically overriding
  the default base image from `.ko.yaml`.

- Add UserAgent to BuildOptions and PublishOptions

  This enables programmatically overriding the `User-Agent` HTTP
  request header for both pulling the base image and pushing the
  built image.

- Rename MakeBuilder to NewBuilder and MakePublisher to NewPublisher.

  For more idiomatic constructor function names.
jonjohnsonjr pushed a commit that referenced this pull request May 25, 2021
- Export functions and a variable to enable embedding of ko's
  `publish` functionality to be embedded in other tools.

  See GoogleContainerTools/skaffold#5611

- Remove DockerRepo PublishOption and flag.

  This removes the `DockerRepo` config option and `--docker-repo`
  flag from the PR.

  New PR with the extracted config option:
  #351

- Fix copyright headers for boilerplate check.

- Use DockerRepo PublishOption instead of env var.

- Override defaultBaseImage using BuildOptions.

  Remove exported package global SetDefaultBaseImage and instead
  allow programmatic override of the default base image using
  the field `BaseImage` in `options.BuildOptions`.

  Also fix copyright header years.

- Add BuildOptions parameter to getBaseImage

  This enables access to BaseImage for programmatically overriding
  the default base image from `.ko.yaml`.

- Add UserAgent to BuildOptions and PublishOptions

  This enables programmatically overriding the `User-Agent` HTTP
  request header for both pulling the base image and pushing the
  built image.

- Rename MakeBuilder to NewBuilder and MakePublisher to NewPublisher.

  For more idiomatic constructor function names.
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.

4 participants