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

✨ Added probe for permissive licenses #3838

Merged
merged 13 commits into from
Apr 12, 2024

Conversation

fhoeborn
Copy link
Contributor

@fhoeborn fhoeborn commented Feb 2, 2024

What kind of change does this PR introduce?

(Is it a bug fix, feature, docs update, something else?)

What is the current behavior?

What is the new behavior (if this is a feature change)?**

  • Tests for the changes have been added (for bug fixes/features)

Which issue(s) this PR fixes

Fixes #3840

Special notes for your reviewer

Does this PR introduce a user-facing change?

For user-facing changes, please add a concise, human-readable release note to
the release-note

(In particular, describe what changes users might need to make in their
application as a result of this pull request.)

Adds a new probe for permissive licenses with the identifier "hasPermissiveLicense"

@fhoeborn fhoeborn requested a review from a team as a code owner February 2, 2024 08:24
@fhoeborn fhoeborn requested review from naveensrinivasan and spencerschrock and removed request for a team February 2, 2024 08:24
@fhoeborn fhoeborn force-pushed the copyleft-check branch 2 times, most recently from 3562d2f to db3bc43 Compare February 2, 2024 08:29
@spencerschrock
Copy link
Contributor

spencerschrock commented Feb 2, 2024

Can you open an issue for discussion? Our contributing.md outlines the steps:
https://github.com/ossf/scorecard/blob/main/checks/write.md#requirements-for-a-check

If you'd like to add a check, make sure it meets the following criteria and then create a new GitHub Issue to discuss with the team:

(there's also some parts about how to implement that are probably out of date now. but I see you have the implementation using probes which is good)

@fhoeborn
Copy link
Contributor Author

fhoeborn commented Feb 2, 2024

Can you open an issue for discussion? Our contributing.md outlines the steps: https://github.com/ossf/scorecard/blob/main/checks/write.md#requirements-for-a-check

If you'd like to add a check, make sure it meets the following criteria and then create a new GitHub Issue to discuss with the team:

(there's also some parts about how to implement that are probably out of date now. but I see you have the implementation using probes which is good)

Thanks for the hint, I opened a issue here #3840

Copy link
Contributor

@cpanato cpanato left a comment

Choose a reason for hiding this comment

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

lgtm

a few nits

checks/evaluation/permissive_license.go Outdated Show resolved Hide resolved
checks/evaluation/permissive_license_test.go Outdated Show resolved Hide resolved
checks/permissive_license.go Outdated Show resolved Hide resolved
checks/evaluation/permissive_license_test.go Outdated Show resolved Hide resolved
checks/permissive_license_test.go Outdated Show resolved Hide resolved
e2e/license_permissive_test.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/def.yml Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/def.yml Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl_test.go Outdated Show resolved Hide resolved
Copy link

github-actions bot commented Mar 6, 2024

This pull request has been marked stale because it has been open for 10 days with no activity

@github-actions github-actions bot added the Stale label Mar 6, 2024
Copy link
Contributor

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

Reviewed the probe portion, which generally looks good.

probes/hasPermissiveLicense/def.yml Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl.go Show resolved Hide resolved
probes/hasPermissiveLicense/impl_test.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl_test.go Outdated Show resolved Hide resolved
@github-actions github-actions bot removed the Stale label Mar 7, 2024
Copy link

This pull request has been marked stale because it has been open for 10 days with no activity

@github-actions github-actions bot added Stale and removed Stale labels Mar 21, 2024
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
fhoeborn and others added 2 commits April 1, 2024 11:35
Signed-off-by: Felix Hoeborn <98820380+fhoeborn@users.noreply.github.com>
…ed tests

Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
@fhoeborn
Copy link
Contributor Author

fhoeborn commented Apr 1, 2024

I adjusted the code a bit based upon your review comments, did you have a chance to look into the rest of the PR?

Copy link
Contributor

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

I adjusted the code a bit based upon your review comments, did you have a chance to look into the rest of the PR?

The reason I focused on the probe code was based on my comment here: #3840 (comment)

I am happy to merge the probe code which is why my review focused on it, but having permissive license as a check is too opinionated (in my opinion) to have as a check for all callers. Full response in the attached issue #3840 (comment)

checks/permissive_license.go Outdated Show resolved Hide resolved
probes/hasPermissiveLicense/impl.go Show resolved Hide resolved
probes/entries.go Outdated Show resolved Hide resolved
…ntly

Signed-off-by: Felix Hoeborn <f.hoeborn@gmail.com>
@fhoeborn
Copy link
Contributor Author

I adjusted the code a bit based upon your review comments, did you have a chance to look into the rest of the PR?

The reason I focused on the probe code was based on my comment here: #3840 (comment)

I am happy to merge the probe code which is why my review focused on it, but having permissive license as a check is too opinionated (in my opinion) to have as a check for all callers. Full response in the attached issue #3840 (comment)

Thanks for the suggestion!
That works fine for this use case (it is a simple true/false probe anyway), I removed the check code and adjusted the probe to be invocated independently

Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
Signed-off-by: Spencer Schrock <sschrock@google.com>
@spencerschrock spencerschrock changed the title ✨ Added check for permissive licenses ✨ Added probe for permissive licenses Apr 12, 2024
Copy link
Contributor

@spencerschrock spencerschrock 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 contribution and being patient / flexible.

Note: for time reasons I went ahead and added a small bit of metadata (related to #4020) and fixed the linter to get this merged before our v5 prerelease candidate (hopefully tomorrow!).

@spencerschrock spencerschrock merged commit 21d53ce into ossf:main Apr 12, 2024
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Feature: New Check "Permissive License"
3 participants