feat: Add downgrade_to option to github_membership resource - #3608
feat: Add downgrade_to option to github_membership resource#3608galargh wants to merge 1 commit into
Conversation
|
👋 Hi, and thank you for this contribution! This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can. You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions. 🤖 This is an automated message. |
|
Project maintainers: this would be really useful for the github organization management situations I have right now. When adjusting membership to my open source organization, I'd like the way to programatically still keep various contributors as collaborators even if I'm freeing up their organization membership seat. This change makes sense to me. What would be the next steps for reviewing and hopefully merging this? |
|
@BigLep while we can't promise timelines or capacity for getting this merged, if ypu want you can support us on this. By looking through our Contributing and Architecture guides and based on those do a review of this PR (or any other in the repo) 🙏 |
| if !d.IsNewResource() { | ||
| ctx = context.WithValue(ctx, ctxId, d.Id()) | ||
| if !d.HasChange("role") { | ||
| return resourceGithubMembershipRead(ctx, d, meta) |
There was a problem hiding this comment.
ARCHITECTURE.md is explicit about this exact function: return nil // Never call Read at end of Update, set any Computed fields in Update. This new branch calls resourceGithubMembershipRead at the end of Update, which also cuts against "Minimize API Calls" in the same doc, since it replaces the PUT it's trying to avoid with a full GET instead. Nothing computed actually changes here when role is unchanged, so return nil (or skipping the block entirely) looks like a better fit for the guide.
On top of that: main just merged #3637 ("Correct legacy etag handling logic"), which added a d.Set("etag", nil) call right before this block in the same function. This branch is behind main so that line isn't in your diff yet, but I merged main into a local copy of this branch to see how the two changes combine, and the etag ends up getting cleared before the HasChange("role") check runs. So even with the Read call kept, an apply that only touches downgrade_on_destroy/downgrade_to throws away the etag first, meaning the read does a full GET instead of a conditional one, losing the caching benefit this resource was just set up for.
resource_github_branch.go's update function has the same Read-at-end-of-Update pattern, so this isn't unprecedented in the codebase, just worth squaring against the doc since it's called out so specifically there. Also curious what motivated adding this skip in the first place, it's not mentioned in the PR description and doesn't look covered by a test yet.
| downgradeOnDestroy := d.Get("downgrade_on_destroy").(bool) | ||
| downgradeTo := "member" | ||
| downgradeTo, ok := d.Get("downgrade_to").(string) | ||
| if !ok || downgradeTo == "" { |
There was a problem hiding this comment.
(nit) downgradeTo, ok := d.Get("downgrade_to").(string) followed by the !ok || downgradeTo == "" fallback: since the schema already sets Default: membershipDowngradeToMember, can this fallback actually be hit? If not, might be worth dropping it, or adding a short note on why it's there.
BigLep
left a comment
BigLep
left a comment
There was a problem hiding this comment.
Note: I am no veteran on this codebase or go in general. I spent time going through it with agent help to better understand and ask questions, and see any callouts missed from https://github.com/integrations/terraform-provider-github/blob/main/CONTRIBUTING.md and https://github.com/integrations/terraform-provider-github/blob/main/ARCHITECTURE.md
The outside_collaborator conversion path is covered well: the mock-based delete tests, the pending-invitation error path, and the acceptance test all check out, and I ran the new unit tests locally, they pass. Two comments below: one on how the new update-skip logic here squares with ARCHITECTURE.md and the etag fix that just landed on main in #3637, and a small question about a defensive fallback in the delete function.
3 participants
Resolves #3607
Before the change?
downgrade_on_destroyparameterAfter the change?
github_membershipgets a new optionaldowngrade_toparametermember, which preserves the previous behaviour in a backwards-compatible manneroutside_collaboratorPull request checklist
Schema migrations have been created if needed (example)(n/a: the proposal is backwards-compatible)Does this introduce a breaking change?
Please see our docs on breaking changes to help!
Testing
Warning
TestAccGithubMembership/downgrades_organization_membership_to_outside_collaboratorrequiresGH_TEST_EXTERNAL_USER1_TOKENto be a Classic PAT withwrite:orgpermissions. This is because only active organisation members can be turned into outside collaborators, so the invite it issues must be accepted. Unfortunately, the fine-grained tokens do not cover organisation invitation acceptance. The newly added behaviour is also covered by unit tests, so we could remove this acceptance test case if we don't want to use a classic token here.✅ Acceptance Tests
TestAccGithubMembership/downgrades_organization_membership_to_outside_collaboratorgithub_membershipresource✅ Unit Tests
Test_resourceGithubMembershipDelete/removes membership- asserts that destroy withdowngrade_on_destroy = falseis successful (uses mocks);Test_resourceGithubMembershipDelete/member default; asserts that destroy withdowngrade_on_destroy = trueis successful (uses mocks);Test_resourceGithubMembershipDelete/outside collaborator; asserts that destroy withdowngrade_on_destroy = trueanddowngrade_to = outside_collaboratoris successful (uses mocks);Test_resourceGithubMembershipDelete/outside collaborator pending invitation- asserts that destroy withdowngrade_on_destroy = trueanddowngrade_to = outside_collaboratorwhen the org invitation has not been accepted is NOT successful (uses mocks);Test_resourceGithubMembershipDelete/membership not found- asserts that destroy withdowngrade_on_destroy = trueanddowngrade_to = outside_collaboratorexits cleanly when the user is not an organisation member (uses mocks);Test_resourceGithubMembershipDowngradeToValidation- verifies validdowngrade_tovalues:memberoutside_collaborator