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

chore: enable policy-bot evaluation #1960

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

strantalis
Copy link
Member

@strantalis strantalis commented Mar 4, 2025

Proposed Changes

For more information on policy-bot spec https://github.com/palantir/policy-bot?tab=readme-ov-file#policyyml-specification

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@strantalis strantalis force-pushed the dspx-641/define-pb-policy branch from 3beac93 to 4cca811 Compare March 4, 2025 13:28
@strantalis strantalis marked this pull request as ready for review March 4, 2025 21:00
@strantalis strantalis requested a review from a team as a code owner March 4, 2025 21:00
eastokes
eastokes previously approved these changes Mar 5, 2025
Copy link
Member

@eastokes eastokes left a comment

Choose a reason for hiding this comment

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

👍
Looks good to me, I'm not totally up to speed on the spec nuances but makes sense at a quick review

Copy link
Member

@jrschumacher jrschumacher left a comment

Choose a reason for hiding this comment

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

@strantalis could we annotate this .policy.yml document to help communicate what the sections mean and where to go for further information.

Copy link
Contributor

@jentfoo jentfoo left a comment

Choose a reason for hiding this comment

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

NODTM with the policy, but please add .policy.yml to the CODEOWNERS. This config can be very sensitive, so we should ensure it's protected.

@strantalis
Copy link
Member Author

strantalis commented Mar 5, 2025

@jentfoo

NODTM with the policy, but please add .policy.yml to the CODEOWNERS. This config can be very sensitive, so we should ensure it's protected.

So I think the goal would be to flatten the codeowners and have this rule enforce an approval from architecture or security if .policy.yml is changed.

https://github.com/opentdf/platform/pull/1960/files#diff-2b2c40c447b8cc3c757b3526d74d04fd8d43e3f17c7ad4f9e6cdfa9bc1fc80d3R141-R158

@strantalis
Copy link
Member Author

@strantalis could we annotate this .policy.yml document to help communicate what the sections mean and where to go for further information.

@jrschumacher I added a link to the configuration. Are you looking for more details than what is put in the descriptions?

@dmihalcik-virtru
Copy link
Member

IMO we should simplify/reduce CODEOWNERs to avoid duplication of effort, but it doesn't seem to me that policy-bot will give us the kind of close integration with tooling we want to fully replace it. Also, I worry about reliability and visibility about using a private hosted service, especially on a public repo

@strantalis
Copy link
Member Author

@dmihalcik-virtru

IMO we should simplify/reduce CODEOWNERs to avoid duplication of effort, but it doesn't seem to me that policy-bot will give us the kind of close integration with tooling we want to fully replace it. Also, I worry about reliability and visibility about using a private hosted service, especially on a public repo

I think we would need to flatten the codeowners but I didn't want to change that until we felt comfortable with using policy-bot. We can also make the service public once we feel comfortable enough with using it.

This shouldn't block any pull requests until we set a required status check.

@pnancarrow
Copy link
Member

but it doesn't seem to me that policy-bot will give us the kind of close integration with tooling we want to fully replace it.

what is policy bot lacking that would give you the close integration you are looking for? Github doesn't natively support much of what can be done with policy bot, so it doesn't seem there is a better option.

@jentfoo
Copy link
Contributor

jentfoo commented Mar 11, 2025

@strantalis I am not sure if putting this restriction in policy bot alone is sufficient. Unless I am mistaken, I believe the referenced policy config will be the one in the PR. The concern is that someone could modify the config as part of their PR to bypass or remove restrictions like these.

I know the config can be referenced from a different repo, and that would address my concerns. Let me know if you know of anything else, but CODEOWNERS seems a straight forward solution. Additionally it's already pretty standard, I would prefer to keep ownership defined in CODEOWNERS.

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.

6 participants