fix: remove vulnerability alerts from state for archived/deleted repos by DrFaust92 · Pull Request #3553 · integrations/terraform-provider-github

DrFaust92 · 2026-07-20T02:11:25Z

Resolves #3369

Before the change?

resourceGithubRepositoryVulnerabilityAlertsRead returned a hard error when the underlying repository was archived or no longer existed:

Error: repository <owner>/<name> is archived, please remove the resource from your configuration

Because this surfaces during refresh, every subsequent plan/apply fails until the resource is manually removed from both config and state — you can't even terraform state rm cleanly while the read errors. Archiving a repo auto-disables its vulnerability alerts on GitHub's side, so the resource is genuinely unmanageable at that point.

The existing 404 branch also set d.SetId("") and then returned diag.Errorf(...); since the SDK discards state changes when Read returns an error, that removal never actually took effect.

After the change?

Read now follows the provider's documented convention (.github/instructions/schema-and-state.instructions.md: "In Read, a 404 from GitHub means 'remove from state' — call d.SetId("") and return nil"):

Alerts 404 (resource gone) → d.SetId("") + return nil (removed the stray diag.Errorf).
Repository no longer exists → d.SetId("") + return nil.
Repository archived → d.SetId("") + return nil, with a tflog.Warn.

Also guards against a nil *http.Response before dereferencing StatusCode.

Net effect: Terraform drops the resource from state gracefully. If the config still declares it on an archived repo, the follow-up plan is non-empty (recreate), and Create still returns the pre-existing cannot enable vulnerability alerts on archived repository error — so users get a clear signal to remove the block, instead of a wedged refresh.

Pull request checklist

Schema migrations have been created if needed
Tests for the changes have been added (reworked errors_when_reading_archived_repository → removes_from_state_when_reading_archived_repository, asserting graceful removal + non-empty plan instead of an error)
Docs have been reviewed and added / updated if needed (added a "Notes" section on archived-repo behavior; regenerated docs)

Does this introduce a breaking change?

Read no longer returns an error for archived/deleted repositories; instead the resource is removed from state. This is a behavior change but not a schema breaking change.

Yes
No

github-actions · 2026-07-20T02:43:16Z

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

The Read function for github_repository_vulnerability_alerts returned a hard error when the underlying repository was archived or no longer existed, wedging every subsequent plan/apply with: repository <name> is archived, please remove the resource from your configuration Per the provider's own Read convention (a 404 means "remove from state"), Read now calls d.SetId("") and returns nil in these cases instead of erroring, so Terraform drops the resource gracefully. Also guards against a nil *http.Response before dereferencing StatusCode, and fixes the existing 404 branch that set the ID to "" but then returned an error (which the SDK discards, so the removal never took effect). Resolves integrations#3369 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

These provider review instructions are being used.

Updates vulnerability-alert refresh behavior so archived or deleted repositories are removed from Terraform state rather than blocking refresh.

Changes:

Gracefully clears state for unavailable alerts and archived/deleted repositories.
Updates acceptance coverage for archived repositories.
Documents archived-repository behavior.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 2 comments.

File	Description
`github/resource_github_repository_vulnerability_alerts.go`	Updates read/error handling.
`github/resource_github_repository_vulnerability_alerts_test.go`	Tests archived-repository refresh.
`templates/resources/repository_vulnerability_alerts.md.tmpl`	Adds behavior notes.
`docs/resources/repository_vulnerability_alerts.md`	Regenerates documentation.

DrFaust92 · 2026-08-01T12:57:49Z

 		repo, _, err := client.Repositories.Get(ctx, owner, repoName)
 		if err != nil {
-			return diag.Errorf("repository doesn't exist anymore, please remove the resource from your configuration: %s", err.Error())
+			tflog.Warn(ctx, "Repository doesn't exist anymore, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})
+			d.SetId("")
+			return nil


Good catch — fixed in 51c87ad. The out-of-band Repositories.Get now captures its response and only clears state when it returns 404; permission, rate-limit, network, and 5xx errors are propagated via diag.Errorf instead of silently dropping valid state and forcing a recreate.

DrFaust92 · 2026-08-01T12:57:52Z

 					Config:             fmt.Sprintf(withAlertsConfig, repoName, true),
 					ExpectNonEmptyPlan: true,


Added in 51c87ad — new removes_from_state_when_repository_deleted acceptance step deletes the repository out of band, then RefreshState: true + ExpectNonEmptyPlan: true asserts the deleted-repo Read path removes the resource from state. Used RefreshState (matching resource_github_branch_default_test.go) rather than a re-apply, since re-applying would recreate the repo and yield an empty plan.

Address Copilot review on integrations#3553: - Repository lookup now clears state only when it returns 404; permission, rate-limit, network, and 5xx errors are propagated instead of silently dropping valid state and forcing a recreate. - Guard resp against nil before reading StatusCode in the debug log, matching the nil guards added elsewhere in Read. - Add a deleted-repository acceptance step (out-of-band delete + refresh + non-empty plan) covering the deleted-repo path.

deiga · 2026-08-01T19:54:42Z

 	vulnerabilityAlertsEnabled, resp, err := client.Repositories.GetVulnerabilityAlerts(ctx, owner, repoName)
 	if err != nil {
-		if resp.StatusCode == http.StatusNotFound {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {


Please use the established pattern for error handling: https://github.com/integrations/terraform-provider-github/blob/main/ARCHITECTURE.md#error-handling

deiga · 2026-08-01T19:55:04Z

 	if err != nil {
-		if resp.StatusCode == http.StatusNotFound {
+		if resp != nil && resp.StatusCode == http.StatusNotFound {
+			tflog.Warn(ctx, "Vulnerability alerts don't exist, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})


Please lowercase log messages

Suggested change

tflog.Warn(ctx, "Vulnerability alerts don't exist, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})

tflog.Warn(ctx, "vulnerability alerts don't exist, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})

deiga · 2026-08-01T19:57:44Z

+	responseStatus := 0
+	if resp != nil {
+		responseStatus = resp.StatusCode
+	}


question: What's the purpose of this?

deiga · 2026-08-01T19:58:08Z

 	// If no error, but the response status code is 404, we need to check if the repository is accessible.
-	if resp.StatusCode == http.StatusNotFound {
-		repo, _, err := client.Repositories.Get(ctx, owner, repoName)
+	if resp != nil && resp.StatusCode == http.StatusNotFound {


question: Is there a case when resp could be nil here?

deiga · 2026-08-01T19:59:13Z

+					// GitHub's side. Read must remove the resource from state
+					// instead of erroring. Because the config still declares the
+					// resource, the follow-up plan is non-empty (Terraform wants
+					// to recreate it) rather than failing the whole run.


suggestion: Let's add a ConfigPlanCheck to verify that it tries to create a new resource

deiga · 2026-08-01T20:00:03Z

+			resource "github_repository" "test" {
+				name       = "%s"
+				visibility = "private"
+				auto_init  = true
+			}


Please use the mustCreateTestRepository function instead of declaring the repo in Terraform code

deiga · 2026-08-01T20:00:50Z

+				{
+					Config: fmt.Sprintf(withAlertsConfig, repoName),
+					ConfigStateChecks: []statecheck.StateCheck{
+						statecheck.ExpectKnownValue("github_repository_vulnerability_alerts.test", tfjsonpath.New("enabled"), knownvalue.Bool(true)),


No need to check for same value as config

Suggested change

statecheck.ExpectKnownValue("github_repository_vulnerability_alerts.test", tfjsonpath.New("enabled"), knownvalue.Bool(true)),

deiga · 2026-08-01T20:01:31Z

+					// Delete the repository out of band. On refresh, the
+					// vulnerability alerts Read hits a deleted repository and must
+					// remove the resource from state instead of erroring, leaving a
+					// non-empty plan (Terraform wants to recreate it).


Please add a plan check here as well for the Create Action

github-actions Bot added the Type: Bug Something isn't working as documented label Jul 20, 2026

DrFaust92 force-pushed the fix-vuln-alerts-archived-state-removal branch from a5959ba to 32b0152 Compare July 23, 2026 09:05

deiga requested a review from Copilot July 23, 2026 09:48

deiga added this to the v6.14.0 milestone Jul 23, 2026

Copilot started reviewing on behalf of deiga July 23, 2026 09:49 View session

Copilot AI reviewed Jul 23, 2026

View reviewed changes

deiga requested changes Aug 1, 2026

View reviewed changes

deiga added the r/repository_vulnerability_alerts label Aug 1, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fix: remove vulnerability alerts from state for archived/deleted repos - #3553

fix: remove vulnerability alerts from state for archived/deleted repos#3553
DrFaust92 wants to merge 2 commits into
integrations:mainfrom
DrFaust92:fix-vuln-alerts-archived-state-removal

DrFaust92 commented Jul 20, 2026

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Uh oh!

DrFaust92 Aug 1, 2026

Uh oh!

DrFaust92 Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

deiga Aug 1, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

		Config: fmt.Sprintf(withAlertsConfig, repoName, true),
		ExpectNonEmptyPlan: true,

	tflog.Warn(ctx, "Vulnerability alerts don't exist, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})
	tflog.Warn(ctx, "vulnerability alerts don't exist, removing resource from state", map[string]any{"owner": owner, "repo_name": repoName})

Uh oh!

Conversation

DrFaust92 commented Jul 20, 2026

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions Bot commented Jul 20, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants