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 global permissions #609

Merged
merged 14 commits into from
Jul 16, 2023
Merged

Add global permissions #609

merged 14 commits into from
Jul 16, 2023

Conversation

etspaceman
Copy link
Collaborator

@etspaceman etspaceman requested a review from armanbilge July 15, 2023 14:46
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

Just a couple nits but looks good to go. I like how this turned out!

Note for posterity: we had some discussion on Discord about whether to explicitly set permissions for the build, publish, site, etc. jobs. The tricky thing is it's hard to know exactly what permissions are required, particularly if the user has added some custom workflow steps. It's a bit annoying because as soon as we specify a few permissions for the things we know we need, we have to specify all the permissions with no way of falling back to the defaults that the user has set in their repository settings. So it's not very compositional.

I'm not entirely convinced how much security benefit it would bring either. So I think opt-in is okay for now, and we can revisit in the future.

@etspaceman etspaceman requested a review from armanbilge July 16, 2023 15:30
@etspaceman
Copy link
Collaborator Author

@armanbilge did one more optimization by moving the string rendering to the Permissions classes. If you don't like that approach, no worries, we can revert the commit.

@armanbilge
Copy link
Member

If you don't like that approach, no worries, we can revert the commit.

Oh yeah, maybe just leave it the way it was. It was fine and I think following the existing style of the project.

@etspaceman
Copy link
Collaborator Author

Reverted

@etspaceman etspaceman requested a review from armanbilge July 16, 2023 16:18
Copy link
Member

@armanbilge armanbilge left a comment

Choose a reason for hiding this comment

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

thanks!

@armanbilge armanbilge merged commit e974029 into typelevel:main Jul 16, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants