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.

Sign up for GitHub

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

Jump to bottom

✨ Token-Permission: Allow top level permissions not defined if all run level permissions are #1356

Merged
merged 9 commits into from
Dec 8, 2021
Merged

✨ Token-Permission: Allow top level permissions not defined if all run level permissions are #1356

merged 9 commits into from
Dec 8, 2021

Conversation

laurentsimon
Copy link
Contributor

We currently give a score of 0 if all job define their permissions but the top level is not.
This PR removes one point if the top level permissions are not defined but the run level ones are all defined.

See #1128 (comment) for more details.

closes #1128

laurentsimon
laurentsimon commented Dec 3, 2021
View reviewed changes
docs/checks.md Outdated Show resolved Hide resolved
laurentsimon
laurentsimon commented Dec 3, 2021
View reviewed changes
docs/checks.md Outdated Show resolved Hide resolved
@laurentsimon laurentsimon temporarily deployed to integration-test December 3, 2021 23:26 Inactive
@laurentsimon laurentsimon enabled auto-merge (squash) December 3, 2021 23:27
@laurentsimon laurentsimon temporarily deployed to integration-test December 3, 2021 23:27 Inactive
@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Integration tests success for
[8ccbcf0]
(https://github.com/ossf/scorecard/actions/runs/1537463516)

@github-actions
Copy link

github-actions bot commented Dec 3, 2021

Integration tests success for
[a93ab73]
(https://github.com/ossf/scorecard/actions/runs/1537465135)

@laurentsimon laurentsimon temporarily deployed to integration-test December 4, 2021 00:20 Inactive
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Integration tests success for
[d443515]
(https://github.com/ossf/scorecard/actions/runs/1537596760)

@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Integration tests success for
[2b6e926]
(https://github.com/ossf/scorecard/actions/runs/1537596969)

@laurentsimon laurentsimon enabled auto-merge (squash) December 4, 2021 03:05
@laurentsimon laurentsimon temporarily deployed to integration-test December 4, 2021 03:05 Inactive
@github-actions
Copy link

github-actions bot commented Dec 4, 2021

Integration tests success for
[9232d59]
(https://github.com/ossf/scorecard/actions/runs/1537950868)

ristomcgehee
ristomcgehee suggested changes Dec 5, 2021
View reviewed changes
Copy link
Contributor

@ristomcgehee ristomcgehee left a comment

Choose a reason for hiding this comment

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

Overall this looks good. Would you mind adding a file to checks/testdata that gets a score of 9?

checks/permissions.go Outdated Show resolved Hide resolved
@laurentsimon
Copy link
Contributor Author

laurentsimon commented Dec 6, 2021 •
edited
Loading

Overall this looks good. Would you mind adding a file to checks/testdata that gets a score of 9?

thanks for catching this, it slipped thru the cracks!

@laurentsimon laurentsimon temporarily deployed to integration-test December 6, 2021 17:23 Inactive
@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Integration tests success for
[57c1b27]
(https://github.com/ossf/scorecard/actions/runs/1545788982)

olivekl
olivekl approved these changes Dec 6, 2021
View reviewed changes
Copy link
Contributor

@olivekl olivekl left a comment

Choose a reason for hiding this comment

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

LGTM, just minor comments to tweak wording.

docs/checks.md Outdated Show resolved Hide resolved
@laurentsimon laurentsimon temporarily deployed to integration-test December 6, 2021 18:12 Inactive
@laurentsimon
Copy link
Contributor Author

@chrismcgehee looks like I still need your LGTM to merge.

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Integration tests success for
[33b5415]
(https://github.com/ossf/scorecard/actions/runs/1545972483)

@github-actions
Copy link

github-actions bot commented Dec 6, 2021

Integration tests success for
[3ad5118]
(https://github.com/ossf/scorecard/actions/runs/1545972846)

@laurentsimon laurentsimon temporarily deployed to integration-test December 7, 2021 15:19 Inactive
@laurentsimon
Copy link
Contributor Author

friendly ping @chrismcgehee can you LGTM?

@laurentsimon
Copy link
Contributor Author

laurentsimon commented Dec 7, 2021 •
edited
Loading

adding @azeemsgoogle as reviewer to see if he can give me another LGTM to merge.

@github-actions
Copy link

github-actions bot commented Dec 7, 2021

Integration tests success for
[19b8c33]
(https://github.com/ossf/scorecard/actions/runs/1550076019)

azeemshaikh38
azeemshaikh38 approved these changes Dec 7, 2021
View reviewed changes
Copy link
Contributor

@azeemshaikh38 azeemshaikh38 left a comment

Choose a reason for hiding this comment

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

LGTM. Left a comment about comment vs. code mismatch.

checks/permissions.go Show resolved Hide resolved
checks/permissions.go Show resolved Hide resolved
@laurentsimon laurentsimon temporarily deployed to integration-test December 8, 2021 00:51 Inactive
@github-actions
Copy link

github-actions bot commented Dec 8, 2021

Integration tests success for
[8d48a31]
(https://github.com/ossf/scorecard/actions/runs/1551960882)

@laurentsimon laurentsimon mentioned this pull request Dec 8, 2021
Closed
2 tasks
@laurentsimon laurentsimon merged commit 6e013cf into ossf:main Dec 8, 2021
@laurentsimon laurentsimon mentioned this pull request Dec 24, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ristomcgehee ristomcgehee ristomcgehee requested changes

@olivekl olivekl olivekl approved these changes

@azeemshaikh38 azeemshaikh38 azeemshaikh38 approved these changes

@naveensrinivasan naveensrinivasan naveensrinivasan approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Token-Permissions check clarification
5 participants
@laurentsimon @naveensrinivasan @azeemshaikh38 @ristomcgehee @olivekl