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

TF-5568 add support for project custom permissions #745

Merged
merged 4 commits into from
Aug 1, 2023

Conversation

rberecka
Copy link
Contributor

@rberecka rberecka commented Jul 25, 2023

Description

Add ability to choose custom access for team projects. Once selected, users can then choose to set a variety of customizable permissions to different levels of access. For example, if I add a team project and set the TeamProjectAccessAddOptions to include access: "custom" I can then also configure different permissions that will apply to either the project itself (project-access) or all workspace in the project (workspace-access).

Authored with @jbonhag

Testing plan

Pull down locally
Run tests below and comment out the skipUnlessBeta(t) flag. Once we have roll out the feature to TFE and remove the feature flag, we'll remove these skips as well.

External links

Output from tests

If you run atlas locally you can run the tests with the ENABLE_BETA=1 environment variable.

$ ENABLE_BETA=1 TFE_ADDRESS=http://localhost:21080/ TFE_TOKEN=<API TOKEN> go test -run TestTeamProjectAccessesAdd
$ ENABLE_BETA=1 TFE_ADDRESS=http://localhost:21080/ TFE_TOKEN=<API TOKEN> go test -run TestTeamProjectAccessesUpdate
...

@rberecka rberecka force-pushed the TF-5568-add-support-for-project-custom-permissions branch from eb9d82b to 253869a Compare July 25, 2023 22:31
@rberecka rberecka marked this pull request as ready for review July 25, 2023 23:10
@rberecka rberecka requested a review from a team as a code owner July 25, 2023 23:10
team_project_access.go Outdated Show resolved Hide resolved
Copy link
Contributor

@jbonhag jbonhag left a comment

Choose a reason for hiding this comment

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

Amazing @rberecka!

team_project_access.go Outdated Show resolved Hide resolved
team_project_access.go Outdated Show resolved Hide resolved
team_project_access_integration_test.go Show resolved Hide resolved
team_project_access_integration_test.go Show resolved Hide resolved
@rberecka rberecka force-pushed the TF-5568-add-support-for-project-custom-permissions branch 5 times, most recently from 7f7ceb4 to d423935 Compare July 26, 2023 18:38
Copy link
Contributor

@jbonhag jbonhag 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 of observations related to the name changes, but this looks really good.

team_project_access.go Show resolved Hide resolved
team_project_access.go Outdated Show resolved Hide resolved
team_project_access_integration_test.go Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
@rberecka rberecka changed the title Tf 5568 add support for project custom permissions TF-5568 add support for project custom permissions Jul 26, 2023
jbonhag
jbonhag previously approved these changes Jul 27, 2023
Copy link
Contributor

@jbonhag jbonhag left a comment

Choose a reason for hiding this comment

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

🎉

CHANGELOG.md Outdated Show resolved Hide resolved
CHANGELOG.md Outdated Show resolved Hide resolved
jbonhag
jbonhag previously approved these changes Jul 27, 2023
@laurenolivia
Copy link
Contributor

@rberecka Great work on this PR ✨

I noticed there is a TeamProjectAccess struct, and a TeamProjectAccesses interface. The naming is almost identical and I'm wondering if it's too late to propose something like a TeamProjectAccessMethods (interface) instead? Especially considering the commented line above the interface reads:

// TeamProjectAccesses describes all the team project access related methods that the Terraform
// Enterprise API supports

Not to mention this is customer-facing. What do you think? How can I support?

@sebasslash
Copy link
Contributor

Unfortunately given go-tfe's v1.0 nature and this interface being exported, we can't rename TeamProjectAccesses to something slightly less confusing. Thankfully from a client perspective you call client.TeamProjectAccess where the naming makes more sense.

@laurenolivia
Copy link
Contributor

Unfortunately given go-tfe's v1.0 nature and this interface being exported, we can't rename TeamProjectAccesses to something slightly less confusing. Thankfully from a client perspective you call client.TeamProjectAccess where the naming makes more sense.

This would be a breaking change for v1.0 but what are your thoughts for v2.0? Or is it not worth it at that point? @sebasslash

@sebasslash
Copy link
Contributor

I think over time if we can identify multiple quirks with our naming conventions as well as other potentially breaking changes that would improve the DX, we can build a good case for a v2.0.

For the time being, v2.0 in go-tfe is in the way distant future.

@laurenolivia
Copy link
Contributor

I think over time if we can identify multiple quirks with our naming conventions as well as other potentially breaking changes that would improve the DX, we can build a good case for a v2.0.

For the time being, v2.0 in go-tfe is in the way distant future.

Fair! Thanks for weighing-in @sebasslash
@rberecka nvm! 👍

Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Awesome work 🔥 ! My comments are mostly regarding code organization and testing

team_project_access.go Outdated Show resolved Hide resolved
team_project_access.go Show resolved Hide resolved
team_project_access.go Outdated Show resolved Hide resolved
team_project_access_integration_test.go Show resolved Hide resolved
team_project_access_integration_test.go Outdated Show resolved Hide resolved
type_helpers.go Show resolved Hide resolved
@rberecka rberecka force-pushed the TF-5568-add-support-for-project-custom-permissions branch 3 times, most recently from 6ed29fe to c8d742c Compare July 31, 2023 17:30
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

This PR has my approval pending the naming changes to scope these permission structs to the parent resource TeamProjectAccess. Awesome work @rberecka !!

I ran the tests locally ✅

@rberecka rberecka force-pushed the TF-5568-add-support-for-project-custom-permissions branch from a83a11c to 4b3e1c7 Compare July 31, 2023 21:53
@rberecka rberecka force-pushed the TF-5568-add-support-for-project-custom-permissions branch from 444c2a5 to a86268b Compare July 31, 2023 22:02
CHANGELOG.md Outdated Show resolved Hide resolved
sebasslash
sebasslash previously approved these changes Aug 1, 2023
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

Approved 🚀

CHANGELOG.md Show resolved Hide resolved
team_project_access.go Outdated Show resolved Hide resolved
Copy link
Contributor

@laurenolivia laurenolivia left a comment

Choose a reason for hiding this comment

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

nit: can we capitalize here

@rberecka rberecka merged commit 9a03826 into main Aug 1, 2023
9 checks passed
@rberecka rberecka deleted the TF-5568-add-support-for-project-custom-permissions branch August 1, 2023 20:09
@github-actions
Copy link

github-actions bot commented Aug 1, 2023

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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.

5 participants