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 full license texts and notice files as possible report columns #515

Merged
merged 5 commits into from
Oct 22, 2018

Conversation

MrBerg
Copy link
Contributor

@MrBerg MrBerg commented Oct 12, 2018

Some licenses require dependent work to display the actual license text, since that can include copyright notices. At least https://www.apache.org/licenses/LICENSE-2.0.html#redistribution also requires displaying any NOTICE files, and in other circles it is common to include the licenses for transitive dependencies there.

@pivotal-so
Copy link
Contributor

Hey there, @MrBerg. Thank you for your contribution thus far. To merge this pull request, we kindly ask that you add the relevant tests for the newly added NoticeFiles class. You may want to refer to license_files_spec.rb for more information.

@MrBerg
Copy link
Contributor Author

MrBerg commented Oct 16, 2018

Sure, @pivotal-dk and @xtreme-vikram-yadav. I pushed some specs now.

Copy link
Collaborator

@Manifaust Manifaust left a comment

Choose a reason for hiding this comment

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

Thanks for the PR, can you take a look at these changes?

spec/fixtures/utf8_gem/NOTICE Outdated Show resolved Hide resolved
Copy link
Contributor

@JennyYJK JennyYJK left a comment

Choose a reason for hiding this comment

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

Thank you for the PR @MrBerg . Please take a look at the change requests.

@@ -149,6 +150,10 @@ def license_files
LicenseFiles.find(install_path, logger: logger)
end

def notice_files
Copy link
Contributor

Choose a reason for hiding this comment

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

@MrBerg can you add test for this function? Please refer to spec/lib/license_finder/package_spec.rb.

Copy link
Contributor Author

@MrBerg MrBerg Oct 18, 2018

Choose a reason for hiding this comment

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

What even needs to be tested here @JennyYJK? E.g. license_files is not tested directly in package_spec.rb, it's just stubbed. NoticeFiles#find is tested in spec/lib/license_finder/packages/notice_files_spec.rb

lib/license_finder/packages/merged_package.rb Show resolved Hide resolved
@cf-osl-bot cf-osl-bot merged commit ab535bd into pivotal:master Oct 22, 2018
@MrBerg MrBerg deleted the add-full-license-text branch October 22, 2018 19:12
@xtreme-shane-lattanzio
Copy link
Contributor

Bumping for webhook trigger.

@pivotal-issuemaster
Copy link

@MrBerg Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

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.

None yet

8 participants