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

[modules] Dependent command packages can now be built with ko #154

Merged
merged 2 commits into from
May 1, 2020

Conversation

dprotaso
Copy link
Contributor

@dprotaso dprotaso commented Apr 30, 2020

Thus if you have a (in)direct command package as a dependency
say myhost.com/go/package/cmd/run you can now publish this
with the following ko command

$  ko publish myhost.com/go/package/cmd/run

This follows go's module/vendor semantics. Thus go1.14 by default will use vendored modules. With go1.13 you need the explicit flags to use vendored deps

$  GOFLAGS="-mod=vendor" ko publish myhost.com/go/package/cmd/run

Subsequently in yaml files you can drop the superfluous prefixes

ie. this (in a non-module world)

apiVersion: batch/v1
kind: Job
spec:
  containers:
  - name: vegeta
    image: knative.dev/serving/vendor/github.com/tsenart/vegeta

becomes this in the module world

apiVersion: batch/v1
kind: Job
spec:
  containers:
  - name: vegeta
    image: github.com/tsenart/vegeta

@dprotaso
Copy link
Contributor Author

cc #152

@imjasonh
Copy link
Member

I'm not sure I understand what this is trying to achieve. Is the effect that you can ko build a non-package main package?

pkg/build/gobuild.go Outdated Show resolved Hide resolved
@dprotaso dprotaso changed the title allow referencing module dependencies [modules] Dependent command packages can now be built with ko Apr 30, 2020
@dprotaso dprotaso marked this pull request as ready for review April 30, 2020 14:59
@dprotaso dprotaso requested a review from imjasonh April 30, 2020 14:59
@dprotaso
Copy link
Contributor Author

whoops needs a rebase - one minute

Thus if you have a (in)direct command package as a dependency
say `myhost.com/go/package/cmd/run` you can now publish this
with the following ko command

  ko publish myhost.com/go/package/cmd/run
@dprotaso
Copy link
Contributor Author

Failure in CI is go1.12

pkg/build/gobuild.go:134: Errorf format %w has unknown verb w

1.12 is EOL so I made #156

@mattmoor
Copy link
Collaborator

@imjasonh If I'm reading the description correctly, this is the ko publish complement of my change.

modules.deps[info.Path] = &info

if info.Main {
modules.main = &info
Copy link
Collaborator

Choose a reason for hiding this comment

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

couldn't we hit N of these? Which one wins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I believe there's only one

Copy link
Contributor Author

Choose a reason for hiding this comment

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

main != package main but specifically "main module" = root module

Copy link
Collaborator

Choose a reason for hiding this comment

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

that's not confusing at all 🙄

@mattmoor
Copy link
Collaborator

Ok, so reading through the change, I want to make sure I understand...

With my change, a vendored main module foo will work if it is written as ko://foo, but what you are doing is dumping all of the referenced modules, so that just foo still works.

Since in ko publish qualify local import turns ./vendor/foo into foo with modules, this unblocks letting ko publish ./vendor/foo or ko publish foo from working.

@dprotaso is that right?

@dprotaso
Copy link
Contributor Author

this unblocks letting ko publish ./vendor/foo or ko publish foo from working.

This unblocks ko publish foo
This does not unblock ko publish ./vendor/foo

@mattmoor
Copy link
Collaborator

mattmoor commented May 1, 2020

I wonder, would simply having qualifyLocalImport slap a ko:// on the front of what it produces work?

@dprotaso
Copy link
Contributor Author

dprotaso commented May 1, 2020

🤷‍♂️

@mattmoor
Copy link
Collaborator

mattmoor commented May 1, 2020

This just makes me want to deprecated non-ko:// faster, but it seems like an improvement of the status quo. thanks for chasing this down.

@mattmoor mattmoor merged commit 6cbfe96 into ko-build:master May 1, 2020
@dprotaso dprotaso deleted the module-ref branch May 1, 2020 14:54
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 this pull request may close these issues.

3 participants