-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
feat(policies): Add granular policy_sets #3086
feat(policies): Add granular policy_sets #3086
Conversation
Right now I'm thinking of re-factoring contest_client.go to pass a list of structs, instead of text output, which can be interpreted by policy_check_runner.go to generate the output there, and also update pull status with additional info about how many approvals a given policyset requires/has received. |
2629f1d
to
8ae774b
Compare
This comment was marked as outdated.
This comment was marked as outdated.
server/events/templates/approve_policies_results_unwrapped.tmpl
Outdated
Show resolved
Hide resolved
I've verified that the core functionality is set up with local testing. It should just be minor tweaking and polishing from here. I'll get to starting on tests and documentation tonight/tomorrow. |
I was thinking more about sticky policy approvals. Would something like storing the commit ID in the Atlantis DB and preserving reviews if the commit IDs match between policy check runs make sense? This behavior would, of course, be gated behind a parameter which defaults to false. |
I'll open a separate issue to encapsulate the above problem and a few other enhancements to policies to take care of once this is merged in. |
Is there a way to re-trigger tests/checks? |
Seems like the build is stuck. |
I have no idea, I guess github could be having issues? |
The PR was in a mergeable state the other day, but something funny is going on now. I'm not quite sure how to resolve. |
@nitrocode @jamengual @GenPage - Apologies for the ping. Any chance we can get this merged in this week? It'd be great to get back on an upstream Atlantis image. |
@pseudomorph no worries, I will take a look today. I'm also fixing some of the workflows. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One final review, everything looks good. It touches a lot of the codebase but I don't see any issues right off the bat. Thank you for the documentation update with pictures. Please make sure to test this with the dev
image tags before we release.
Thank you so much! I'll get us on the dev image in the next week. |
* Initial work. * Periodic push. * Fmt and start adding args to approve_policies cmd. * keep funcs for now. * Periodic push. * Periodic push. * fmt. * Move approve policies logic to project_command_runner. * update some tests * More test fixes. * update more tests. fix som logic. * more tests. add additional info to common data for custom templates. * fix apply with policies bug. update more tests/fmt * file perms * fix error parsing for conftest results. * Update more tests and linting. * update documentation. * Address no-fail case. Address comments. * Forgot changes. * fix markdown renderer * Fix policy fail logic. remove uneeded tmpl var * targeted policy approvals fix * Address PR comments. * empty commit to trigger build --------- Co-authored-by: PePe Amengual <jose.amengual@gmail.com> Co-authored-by: rkstrickland <ross.strickland@instacart.com> Co-authored-by: Dylan Page <dylan.page@autodesk.com>
* Initial work. * Periodic push. * Fmt and start adding args to approve_policies cmd. * keep funcs for now. * Periodic push. * Periodic push. * fmt. * Move approve policies logic to project_command_runner. * update some tests * More test fixes. * update more tests. fix som logic. * more tests. add additional info to common data for custom templates. * fix apply with policies bug. update more tests/fmt * file perms * fix error parsing for conftest results. * Update more tests and linting. * update documentation. * Address no-fail case. Address comments. * Forgot changes. * fix markdown renderer * Fix policy fail logic. remove uneeded tmpl var * targeted policy approvals fix * Address PR comments. * empty commit to trigger build --------- Co-authored-by: PePe Amengual <jose.amengual@gmail.com> Co-authored-by: rkstrickland <ross.strickland@instacart.com> Co-authored-by: Dylan Page <dylan.page@autodesk.com>
* Initial work. * Periodic push. * Fmt and start adding args to approve_policies cmd. * keep funcs for now. * Periodic push. * Periodic push. * fmt. * Move approve policies logic to project_command_runner. * update some tests * More test fixes. * update more tests. fix som logic. * more tests. add additional info to common data for custom templates. * fix apply with policies bug. update more tests/fmt * file perms * fix error parsing for conftest results. * Update more tests and linting. * update documentation. * Address no-fail case. Address comments. * Forgot changes. * fix markdown renderer * Fix policy fail logic. remove uneeded tmpl var * targeted policy approvals fix * Address PR comments. * empty commit to trigger build --------- Co-authored-by: PePe Amengual <jose.amengual@gmail.com> Co-authored-by: rkstrickland <ross.strickland@instacart.com> Co-authored-by: Dylan Page <dylan.page@autodesk.com>
* Initial work. * Periodic push. * Fmt and start adding args to approve_policies cmd. * keep funcs for now. * Periodic push. * Periodic push. * fmt. * Move approve policies logic to project_command_runner. * update some tests * More test fixes. * update more tests. fix som logic. * more tests. add additional info to common data for custom templates. * fix apply with policies bug. update more tests/fmt * file perms * fix error parsing for conftest results. * Update more tests and linting. * update documentation. * Address no-fail case. Address comments. * Forgot changes. * fix markdown renderer * Fix policy fail logic. remove uneeded tmpl var * targeted policy approvals fix * Address PR comments. * empty commit to trigger build --------- Co-authored-by: PePe Amengual <jose.amengual@gmail.com> Co-authored-by: rkstrickland <ross.strickland@instacart.com> Co-authored-by: Dylan Page <dylan.page@autodesk.com>
what
This change updates policy checks to be more granular.
Namely:
Implementation details:
PolicySet configuration allows for the specifying of owners at the global level and on a per policy set level. Additionally, a new parameter
ReviewCount
, which specifies how many approvals are required to override policy checks, has been added on the same levels. These are both considered when determining if a policy set is approved or not.Some changes were made to pass around or expose the additional context needed to manage state between policy_approval commands. The BoltDB/Redis backends have been updated to store information about each policy set on a per-project level.
Since top-level policy set owners can approve all defined policy sets, an additional
--policy-set
argument has been added to the comment command parser, which allows for targeted policy approvals. Along with this change, policies can be approved on a per plan, workspace, or dir level, using the same args as all other Atlantis commands.Having discreetly approvable policy sets changes the possible error/success dynamic that the markdown renderer used to rely on.
For example:
A PR might have two failing policy sets. Someone comes along to approve and is authorized for one but not the other. In this case, we want to bubble up both the successful approval one policy set (or at least information about the approval state) along with an error indicating the authorization issue on the other.
This would also be the case where a user is authorized for all policy sets, but the policy set requires more than one approval.
To account for this, I've updated the markdown renderer to render all policies before error or failure, and then if there is error or failure, the rendered content is passed into those templates as additional context.
why
Currently, when multiple policy sets are specified, only one set of owners is considered for all, and only one approval is sufficient for overriding policy checks, where multiple might be desired.
tests
Updated existing tests, and added some new ones.
references