Detect not-found via the error (errors.AsType) in github_team_repository delete by rilical · Pull Request #3524 · integrations/terraform-provider-github

rilical · 2026-07-06T13:47:54Z

Resolves #3523

Follow-up to #3518 (now merged), addressing @stevehipwell's review feedback.

What changed

resourceGithubTeamRepositoryDelete now detects the not-found case from the returned error via errors.AsType[*github.ErrorResponse](err) instead of reading resp.StatusCode off the raw response, restructured to check the error first (matching the delete style in resource_github_issue_label.go).
Swept the redundant Response != nil guard on *github.ErrorResponse across the codebase, since a *github.ErrorResponse always carries a non-nil Response (go-github only builds one in CheckResponse): util_retry.go, provider.go, and resource_github_enterprise_organization.go.
Normalized util_retry.go's not-found check from the literal 404 to http.StatusNotFound.

Behavior-preserving: the repo-rename retry and archived/404 handling are unchanged, and the nil-response panic regression test added in #3518 still covers this delete path.

Open question: a bare 404 on delete still returns the error rather than being idempotent (return nil, like resource_github_issue_label.go), and the rename retry is still there. Happy to simplify either if you'd like, I just didn't want to change destroy semantics unprompted.

Verification

go build, go vet, gofmt clean.
Unit suite green (make test).
Live TestAccGithubTeamRepository passes (create / read / import / update / destroy).

Pull request checklist

Schema migrations have been created if needed (N/A, no schema change)
Tests for the changes have been added (delete path stays covered by Fix nil-pointer panic when deleting github_team_repository #3518's regression test; no new test per reviewer preference)
Docs have been reviewed and added / updated if needed (N/A, no user-facing change)

Does this introduce a breaking change?

Yes
No

AI-assisted disclosure: developed with AI assistance (GitHub Copilot). Per the repository's AI Use Policy I have reviewed and understand the change, verified the errors.AsType + *github.ErrorResponse invariant against go-github v88's construction sites and the provider's usages, and tested it with the unit regression suite plus a live acceptance run of TestAccGithubTeamRepository.

github-actions · 2026-07-06T13:48:04Z

👋 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.}

stevehipwell

I've added a couple of inline comments, could you also look at the rest of the codebase for this pattern and make the relevant changes?

stevehipwell · 2026-07-06T13:52:01Z

+	_, err = client.Teams.RemoveTeamRepoByID(ctx, orgId, teamId, orgName, repoName)

-	if resp.StatusCode == 404 {
+	if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response != nil && ghErr.Response.StatusCode == http.StatusNotFound {


Suggested change

if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response != nil && ghErr.Response.StatusCode == http.StatusNotFound {

if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response.StatusCode == http.StatusNotFound {

The response nil check is unnecessary as the type is a github.ErrorResponse and the response is always set (the pointer is to stop the struct being copied). Could you also remove where this pattern has been added to the codebase due to a Copilot review that I didn't fully validate.

Understood, on it.

Done. Removed the nil check here and swept the same redundant guard everywhere it had crept in from earlier Copilot reviews:

github/util_retry.go (retryUntilResourceFound)

github/provider.go (the org lookup, which had the inverted Response == nil || form)

github/resource_github_enterprise_organization.go (isSAMLEnforcementError)

You're right that it's dead code: go-github only ever builds *github.ErrorResponse in CheckResponse as &ErrorResponse{Response: r} with a non-nil r, and the one place we construct it by hand (resource_github_team_members.go) sets Response too. Every other .Response.StatusCode deref in the package already skips the guard, so this just makes these consistent.

I also reworked the delete block to check for the error first and then use errors.AsType (matching resource_github_issue_label.go), and switched util_retry's literal 404 to http.StatusNotFound.

One thing I left alone: a bare 404 with no rename still returns the error through handleArchivedRepoDelete rather than being treated as idempotent (return nil) like issue_label/team_members, and I kept the repo-rename retry. Happy to make 404 a clean no-op and drop the rename lookup if that's the direction you want, just didn't want to change destroy semantics without a nod.

stevehipwell · 2026-07-06T13:53:13Z

+// recovery branch, which issues a follow-up GET for the repository. Observing
+// that second request proves the error-based 404 detection fired; if detection
+// regressed, only the single delete request would be made.
+func Test_resourceGithubTeamRepositoryDelete_notFoundTriggersRepositoryLookup(t *testing.T) {


I'm not 100% sure we need a test for these as we're going to see the same pattern in all of the resources.

Removed it. I kept the panic regression test from #3518 (Test_resourceGithubTeamRepositoryDelete_nilResponseDoesNotPanic) since that covers a different path (a nil *Response on a transport error), but dropped the not-found lookup test since, like you said, that same pattern repeats across every resource.

rilical · 2026-07-06T14:48:04Z

sorry for the force-push, I accidentally committed a local state file (.pulsar-state.json) in the last push. dropped it, no code changes.

stevehipwell · 2026-07-06T16:07:01Z

@rilical do you want to rebase again?

…ory delete Detect the not-found case in resourceGithubTeamRepositoryDelete from the returned error via errors.AsType[*github.ErrorResponse] instead of reading resp.StatusCode off the raw response, and restructure to check the error first, matching the delete style in resource_github_issue_label.go. The repository-rename recovery and archived-repository handling are unchanged. A *github.ErrorResponse always carries a non-nil Response (go-github only builds one in CheckResponse), so drop the redundant Response != nil guard here and at the other sites that had picked up the same pattern: util_retry.go, provider.go, and resource_github_enterprise_organization.go. Normalize util_retry.go's not-found check from the literal 404 to http.StatusNotFound. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

rilical · 2026-07-06T16:50:29Z

yep, done. rebased onto main now that #3518 landed, so it's down to just the single refactor commit. also refreshed the description to match.

deiga

Looking at go-github it seems to me that a json.Unmarshal error can result in an github.ErrorResponse with a nil Response attribute. I don't think it makes sense to make the providers code more brittle by removing this nil checks

deiga · 2026-07-07T18:22:38Z

+			repo, _, err := client.Repositories.Get(ctx, orgName, repoName)
+			if err != nil {
+				return diag.FromErr(err)
+			}
+			newRepoName := repo.GetName()
+			if newRepoName != repoName {
+				log.Printf("[INFO] Repo name has changed %s -> %s. "+
+					"Try deleting team repository again.",
+					repoName, newRepoName)
+				_, err := client.Teams.RemoveTeamRepoByID(ctx, orgId, teamId, orgName, newRepoName)


issue: all of this is the unnecessary complexity Steve was talking about. This needs to go and the resource needs to be refactored to utilize the similar team renamed pattern as the repo renamed pattern being used

Done, this is gone. Dropped the Repositories.Get + rename-and-retry block entirely and simplified delete to the same idempotent shape as resource_github_issue_label.go: a 404 from RemoveTeamRepoByID now returns nil (already gone), and anything else flows through the existing archived-repo handling. Behavior change worth calling out: a bare 404 on delete is now a no-op instead of surfacing an error. The #3518 nil-response panic test still covers the transport-error path and passes.

Per review, drop the Repositories.Get lookup and the rename-and-retry branch from resourceGithubTeamRepositoryDelete and simplify to the idempotent delete pattern used by resource_github_issue_label.go: a 404 from RemoveTeamRepoByID is treated as already-deleted (return nil), and any other error flows through handleArchivedRepoDelete as before. Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com>

rilical · 2026-07-07T18:57:34Z

@deiga thanks for taking a look. I went through go-github v88 (the version pinned in go.mod) and I don't think that path exists there:

ErrorResponse.Response is tagged json:"-" (github/github.go:1441), so json.Unmarshal never populates or clears it.
CheckResponse builds it as &ErrorResponse{Response: r} with a non-nil r, and on a json.Unmarshal error it explicitly rebuilds it the same way, errorResponse = &ErrorResponse{Response: r}, with the comment // reset the response as if this never happened (github/github.go:1727-1734). So the unmarshal-error case you describe still carries a non-nil Response. The default branch returns that same value; the only other branches return *RateLimitError/*AbuseRateLimitError/*RedirectionError.
go-github relies on this itself: parseBoolResponse dereferences rerr.Response.StatusCode with no nil check (github/github.go:1800).

Every *github.ErrorResponse dereference already in this provider skips the guard (e.g. resource_github_issue_label.go), and the one place we build one by hand sets Response too. So for errors coming off the go-github client, which is all of these call sites, Response is always non-nil and the guard is dead code rather than protection, which is why I dropped it per Steve's earlier request. If you're seeing a construction path I've missed, point me at it and I'll gladly restore the checks.

deiga · 2026-07-08T05:19:27Z

@rilical ah, Sorry. I didn't see Steve's comment. Yes, then the nil checks are safe to remove.

stevehipwell

LGTM

stevehipwell added this to the v6.14.0 milestone Jul 6, 2026

stevehipwell added the Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR label Jul 6, 2026

stevehipwell requested changes Jul 6, 2026

View reviewed changes

rilical force-pushed the refactor-team-repository-delete-error-handling branch from 454bbc7 to 1b9a459 Compare July 6, 2026 14:47

rilical force-pushed the refactor-team-repository-delete-error-handling branch from 1b9a459 to cd9ce48 Compare July 6, 2026 16:49

rilical requested a review from stevehipwell July 7, 2026 17:26

deiga requested changes Jul 7, 2026

View reviewed changes

stevehipwell approved these changes Jul 8, 2026

View reviewed changes

	if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response != nil && ghErr.Response.StatusCode == http.StatusNotFound {
	if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok && ghErr.Response.StatusCode == http.StatusNotFound {

Uh oh!

Conversation

rilical commented Jul 6, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changed

Verification

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions Bot commented Jul 6, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

stevehipwell Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

rilical Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

rilical Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

stevehipwell Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

rilical Jul 6, 2026

Choose a reason for hiding this comment

Uh oh!

rilical commented Jul 6, 2026

Uh oh!

stevehipwell commented Jul 6, 2026

Uh oh!

rilical commented Jul 6, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

deiga Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rilical Jul 7, 2026

Choose a reason for hiding this comment

Uh oh!

rilical commented Jul 7, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

deiga commented Jul 8, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rilical commented Jul 6, 2026 •

edited

Loading

rilical commented Jul 7, 2026 •

edited

Loading