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

feat: Support team owners for conftest policies #2953

Merged
merged 1 commit into from Jan 10, 2023

Conversation

DazWorrall
Copy link
Contributor

@DazWorrall DazWorrall commented Jan 9, 2023

what

In addition to a list of usernames, support checking team membership when validating a particular user can approve policy violations. The new code path is only executed if team membership is configured, so it's a low impact change for existing installations.

This also documents a missing GitHub app permission - GetTeamNamesForUser was added in #1694 and requires the members:read scope, I had to add that to my GitHub app to test this feature.

I need to make some doc updates for this, but wanted to get some feedback on the code first. docs updated

why

Maintaining a list of users by hand is a lot of toil for larger orgs.

tests

  • Tested against Github using a real GitHub app, org, and repo. (gif)
  • Unit tests

references

Copy link
Contributor

@krrrr38 krrrr38 left a comment

Choose a reason for hiding this comment

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

The current implementation looks good to me 👍 (please add the more test)

teams := []string{}

// Only query the users team membership if any teams have been configured as owners.
if prjCmds[0].PolicySets.HasTeamOwners() {
Copy link
Contributor

@krrrr38 krrrr38 Jan 9, 2023

Choose a reason for hiding this comment

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

nice check 👍

📝 if teams setting is empty, not call api. so no breaking changes even if existed github-app doesn't have members:read permission.

@krrrr38
Copy link
Contributor

krrrr38 commented Jan 9, 2023

If possible, please add teams field into server-side-repo-config Owner section

Once you edit this file and add into this PR, you can see preview from the github-action deploy/netlify.

Copy link
Contributor Author

@DazWorrall DazWorrall left a comment

Choose a reason for hiding this comment

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

@@ -57,6 +57,8 @@ GitHub App needs these permissions. These are automatically set when a GitHub ap

::: tip NOTE
Since v0.19.7, a new permission for `Administration` has been added. If you have already created a GitHub app, updating Atlantis to v0.19.7 will not automatically add this permission, so you will need to set it manually.

Since v0.22.3, a new permission for `Members` has been added, which is required for features that apply permissions to an organizations team members rather than individual users. Like the `Administration` permission above, updating Atlantis will not automatically add this permission, so if you wish to features that rely on checking team membership you will need to add this manually.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

v0.22.3

This is the next patch release number, not sure if this will end up being the correct version.

@nitrocode nitrocode added this to the 0.22.3 milestone Jan 10, 2023
@nitrocode nitrocode changed the title Support team owners for policies feat: Support team owners for conftest policies Jan 10, 2023
@DazWorrall
Copy link
Contributor Author

@nitrocode is this still needs tests or are you happy with the coverage?

Copy link
Contributor

@jamengual jamengual left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@krrrr38 krrrr38 left a comment

Choose a reason for hiding this comment

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

thank you for your nice patch!

Copy link
Member

@nitrocode nitrocode 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. Thank you for the contribution and thank you all for helping in reviews!

@nitrocode nitrocode merged commit 7746655 into runatlantis:main Jan 10, 2023
@sophhu
Copy link

sophhu commented Apr 17, 2024

sorry if this is a dumb question but this works for teams under an org right? like i can do "owners": {
"teams": [
"@org/team-name"
]
},

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
needs tests Change requires tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support GitHub Teams for conftest policies
5 participants