Skip to content
This repository was archived by the owner on Sep 5, 2019. It is now read-only.

include licenses in all published containers#440

Merged
knative-prow-robot merged 1 commit intoknative:masterfrom
dprotaso:licenses
Oct 25, 2018
Merged

include licenses in all published containers#440
knative-prow-robot merged 1 commit intoknative:masterfrom
dprotaso:licenses

Conversation

@dprotaso
Copy link
Copy Markdown
Member

/hold

@imjasonh
Copy link
Copy Markdown
Contributor

Do we need to include licenses in containers we happen to build during tests, which aren't expected to be publicly available (e.g., test/panic)

cmd/logs isn't built into a container AFAIK, it's only provided as a convenience binary for testing logs integrations. Do we still need license files for that?

nop definitely needs this at least.

Can we add a test that we keep publishing updated licenses for the images that need them? I'm positive I will forget to keep these up to date without a computer helping me.

@dprotaso
Copy link
Copy Markdown
Member Author

Do we need to include licenses in containers we happen to build during tests, which aren't expected to be publicly available (e.g., test/panic)

I could imagine these tests are part of a 'smoke' test that's run after some on-prem installation of Knative.

cmd/logs isn't built into a container AFAIK, it's only provided as a convenience binary for testing logs integrations. Do we still need license files for that?

Probably not - I'll remove it

Can we add a test that we keep publishing updated licenses for the images that need them? I'm positive I will forget to keep these up to date without a computer helping me.

They're symlink'd to point to the root LICENSE file and third_party/VENDOR-LICENSE.

This separate PR #441 addresses keeping those up to date.

@imjasonh
Copy link
Copy Markdown
Contributor

I could imagine these tests are part of a 'smoke' test that's run after some on-prem installation of Knative.

Good point. 👍

They're symlink'd to point to the root LICENSE file and third_party/VENDOR-LICENSE.

This separate PR #441 addresses keeping those up to date.

That's definitely helpful, I'm still a bit worried I'll add a new image and not think to add these symlinks. Should ko be responsible for building/including these license files for us, using dep-collector etc? Or is that squarely outside of its wheelhouse.

cc @mattmoor

@dprotaso
Copy link
Copy Markdown
Member Author

dprotaso commented Oct 24, 2018

That's definitely helpful, I'm still a bit worried I'll add a new image and not think to add these symlinks. Should ko be responsible for building/including these license files for us, using dep-collector etc? Or is that squarely outside of its wheelhouse.

I'd say it falls under the verification/presubmit - ie. scan for package main and see if there's a kodata folder with a LICENSE and VENDOR-LICENSE file

I'd make that a separate PR/issue

@dprotaso
Copy link
Copy Markdown
Member Author

I see now - the generic approach with presubmit/verification would flag cmd/logs as needing a kodata folder

@dprotaso
Copy link
Copy Markdown
Member Author

Maybe this is a use case for an extension?
google/go-containerregistry#292

@dprotaso
Copy link
Copy Markdown
Member Author

/hold cancel

@knative-prow-robot
Copy link
Copy Markdown

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dprotaso, shashwathi

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@imjasonh
Copy link
Copy Markdown
Contributor

/lgtm

@knative-prow-robot knative-prow-robot merged commit 98a3e1b into knative:master Oct 25, 2018
@dprotaso dprotaso deleted the licenses branch October 26, 2018 14:04
mgencur pushed a commit to mgencur/knative-build that referenced this pull request Nov 13, 2018
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
vdemeester pushed a commit to vdemeester/knative-build that referenced this pull request Apr 3, 2019
@dprotaso
Copy link
Copy Markdown
Member Author

gubernator 👊

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants