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

Generate notice file from dependency licences #1764

Merged
merged 5 commits into from
Sep 25, 2019

Conversation

charith-elastic
Copy link
Contributor

An alternative implementation of #1689 that does not require generating the vendor directory. This code parses the output of go list -m -json all to locate all dependencies and their paths on disk (taking replace directives into account). It then detects possible licence files in the path and builds a notice file.

Sample output generated using the output of go list -m -json all on Kubebuilder v2 branch: https://gist.github.com/charith-elastic/fa284284cfbcdfa479820f28d542fc48

@charith-elastic charith-elastic marked this pull request as ready for review September 24, 2019 07:45
@barkbay
Copy link
Contributor

barkbay commented Sep 24, 2019

I'm not able to run the unit tests:

go test  ./...
?   	github.com/elastic/cloud-on-k8s/hack/licence-detector	[no test files]
--- FAIL: TestDetect (0.00s)
    --- FAIL: TestDetect/All (0.00s)
        detector_test.go:47:
            	Error Trace:	detector_test.go:47
            	Error:      	Received unexpected error:
            	            	unexpected error while finding licence for github.com/ekzhu/minhash-lsh in testdata/github.com/ekzhu/[email protected]: lstat testdata/github.com/ekzhu/[email protected]: no such file or directory
            	Test:       	TestDetect/All
    --- FAIL: TestDetect/DirectOnly (0.00s)
        detector_test.go:47:
            	Error Trace:	detector_test.go:47
            	Error:      	Received unexpected error:
            	            	unexpected error while finding licence for github.com/ekzhu/minhash-lsh in testdata/github.com/ekzhu/[email protected]: lstat testdata/github.com/ekzhu/[email protected]: no such file or directory
            	Test:       	TestDetect/DirectOnly
FAIL
FAIL	github.com/elastic/cloud-on-k8s/hack/licence-detector/detector	0.014s
FAIL

Also should they not be run by the CI ?

hack/licence-detector/detector/detector.go Outdated Show resolved Hide resolved
@charith-elastic
Copy link
Contributor Author

charith-elastic commented Sep 24, 2019

I'm not able to run the unit tests:

One of the test directories was ignored by git because it didn't contain anything (intentionally). It's been fixed now.

Also should they not be run by the CI ?

The tests were mainly for my benefit. This bit of code has nothing to do with the operator so I kept it as a separate project with its own set of dependencies so that we could move it out at a later time if we wanted to.

@barkbay
Copy link
Contributor

barkbay commented Sep 24, 2019

The tests were mainly for my benefit. This bit of code has nothing to do with the operator

It has nothing to do with the operator from a technical point of view, but I think it still has the responsibility to produce a notice with the right content.
If the tests are not run by our CI it means that we have to trust team members that will maintain it.

@charith-elastic
Copy link
Contributor Author

The notice file gets updated every time make generate is run and will be part of the PR so I think it'll be obvious if something has gone wrong.

I don't feel that CI will bring much benefit due to the following reasons:

  • This code should rarely change in practice
  • The tests are synthetic and only cover the detection logic (no guarantee that the result is correct for the main project)
  • Almost all the other tools in the hack directory don't have any tests

If you do feel strongly about it, I am happy to discuss offline.

@barkbay
Copy link
Contributor

barkbay commented Sep 25, 2019

I'm not sure I'm strongly opinionated on this, but given that the generated file seems to be important I would prefer to let someone else approve the fact that we are not running the unit tests.

@barkbay barkbay dismissed their stale review September 25, 2019 11:01

Removed to not block this PR

Copy link
Collaborator

@pebrc pebrc left a comment

Choose a reason for hiding this comment

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

LGTM, clever idea to leverage go list

  • I am +1 on running the tests given that you have written them (I know we have been a bit sloppy in the hack directory)
  • but I don't think it should delay this from being merged

{{ end }}
{{- end -}}

Copyright 2014-{{ currentYear }} Elasticsearch BV
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think I need some help here: why the specific range start of 2014?

Copy link
Contributor Author

@charith-elastic charith-elastic Sep 25, 2019

Choose a reason for hiding this comment

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

I took the header from #1689 -- which I think came from the Beats notice file. Should we change it to when the ECK project was started? (2018 I assume)

@charith-elastic charith-elastic merged commit 259ec80 into elastic:master Sep 25, 2019
@charith-elastic charith-elastic deleted the notice-gen branch September 25, 2019 12:57
@anyasabo
Copy link
Contributor

I wonder if this is worth splitting out into a separate project since anyone using projects with notice requirements has the same need.

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