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

include licenses in all published containers #440

Merged
merged 1 commit into from
Oct 25, 2018

Conversation

dprotaso
Copy link
Member

/hold

@imjasonh
Copy link
Member

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
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
Member

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
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
Member Author

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

@dprotaso
Copy link
Member Author

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

@dprotaso
Copy link
Member Author

/hold cancel

@knative-prow-robot
Copy link

[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

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
Member

/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
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