Add destroy-on-drift property to the GitHub Action Secret resource schema

Author: nickfloyd

github/resource_github_actions_secret.go

@@ -62,6 +62,12 @@ func resourceGithubActionsSecret() *schema.Resource {
 				Computed:    true,
 				Description: "Date of 'actions_secret' update.",
 			},
+			"destroy_on_drift": {
+				Type:        schema.TypeBool,
+				Default:     true,
+				Optional:    true,
+				Description: "Boolean indicating whether to recreate the secret if it's modified outside of Terraform. When `true` (default), Terraform will delete and recreate the secret if it detects external changes. When `false`, Terraform will acknowledge external changes but not recreate the secret.",
+			},
 		},
 	}
 }
@@ -155,9 +161,15 @@ func resourceGithubActionsSecretRead(d *schema.ResourceData, meta interface{}) e
 	// The only solution to enforce consistency between is to mark the resource
 	// as deleted (unset the ID) in order to fix potential drift by recreating
 	// the resource.
-	if updatedAt, ok := d.GetOk("updated_at"); ok && updatedAt != secret.UpdatedAt.String() {
+	destroyOnDrift := d.Get("destroy_on_drift").(bool)
+	if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != secret.UpdatedAt.String() {
 		log.Printf("[INFO] The secret %s has been externally updated in GitHub", d.Id())
 		d.SetId("")
+	} else if updatedAt, ok := d.GetOk("updated_at"); ok && !destroyOnDrift && updatedAt != secret.UpdatedAt.String() {
+		log.Printf("[INFO] The secret %s has been externally updated in GitHub, updating timestamp without recreating", d.Id())
+		if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
+			return err
+		}
 	} else if !ok {
 		if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
 			return err

github/resource_github_actions_secret_test.go

@@ -7,8 +7,8 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/acctest"
-
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
 func TestAccGithubActionsSecret(t *testing.T) {
@@ -295,4 +295,246 @@ func TestAccGithubActionsSecret(t *testing.T) {
 		})
 
 	})
+
+	t.Run("respects destroy_on_drift setting", func(t *testing.T) {
+		randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
+		
+		config := fmt.Sprintf(`
+			resource "github_repository" "test" {
+				name = "tf-acc-test-%s"
+			}
+
+			resource "github_actions_secret" "with_drift_true" {
+				repository        = github_repository.test.name
+				secret_name       = "test_drift_true"
+				plaintext_value   = "initial_value"
+				destroy_on_drift  = true
+			}
+
+			resource "github_actions_secret" "with_drift_false" {
+				repository        = github_repository.test.name
+				secret_name       = "test_drift_false"
+				plaintext_value   = "initial_value"
+				destroy_on_drift  = false
+			}
+
+			resource "github_actions_secret" "default_behavior" {
+				repository        = github_repository.test.name
+				secret_name       = "test_default"
+				plaintext_value   = "initial_value"
+				# destroy_on_drift defaults to true
+			}
+		`, randomID)
+
+		testCase := func(t *testing.T, mode string) {
+			resource.Test(t, resource.TestCase{
+				PreCheck:  func() { skipUnlessMode(t, mode) },
+				Providers: testAccProviders,
+				Steps: []resource.TestStep{
+					{
+						Config: config,
+						Check: resource.ComposeTestCheckFunc(
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.with_drift_true", "destroy_on_drift", "true"),
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.with_drift_false", "destroy_on_drift", "false"),
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.default_behavior", "destroy_on_drift", "true"),
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.with_drift_true", "plaintext_value", "initial_value"),
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.with_drift_false", "plaintext_value", "initial_value"),
+							resource.TestCheckResourceAttr(
+								"github_actions_secret.default_behavior", "plaintext_value", "initial_value"),
+						),
+					},
+				},
+			})
+		}
+
+		t.Run("with an anonymous account", func(t *testing.T) {
+			t.Skip("anonymous account not supported for this operation")
+		})
+
+		t.Run("with an individual account", func(t *testing.T) {
+			testCase(t, individual)
+		})
+
+		t.Run("with an organization account", func(t *testing.T) {
+			testCase(t, organization)
+		})
+	})
+}
+
+// Unit tests for drift detection behavior
+func TestGithubActionsSecretDriftDetection(t *testing.T) {
+	
+	t.Run("destroyOnDrift true causes recreation on timestamp mismatch", func(t *testing.T) {
+		originalTimestamp := "2023-01-01T00:00:00Z"
+		newTimestamp := "2023-01-02T00:00:00Z"
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":       "test-repo",
+			"secret_name":      "test-secret",
+			"plaintext_value":  "test-value",
+			"destroy_on_drift": true,
+			"updated_at":       originalTimestamp,
+		})
+		d.SetId("test-secret")
+
+		// Test the drift detection logic - simulate what happens in the read function
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != newTimestamp {
+			d.SetId("") // This simulates the drift detection
+		}
+
+		// Should have cleared the ID (marking for recreation)
+		if d.Id() != "" {
+			t.Error("Expected ID to be cleared due to drift detection, but it wasn't")
+		}
+	})
+
+	t.Run("destroyOnDrift false updates timestamp without recreation", func(t *testing.T) {
+		originalTimestamp := "2023-01-01T00:00:00Z"
+		newTimestamp := "2023-01-02T00:00:00Z"
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":       "test-repo", 
+			"secret_name":      "test-secret",
+			"plaintext_value":  "test-value",
+			"destroy_on_drift": false,
+			"updated_at":       originalTimestamp,
+		})
+		d.SetId("test-secret")
+
+		// Test the drift detection logic when destroy_on_drift is false
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if updatedAt, ok := d.GetOk("updated_at"); ok && !destroyOnDrift && updatedAt != newTimestamp {
+			// This simulates what happens when destroy_on_drift=false
+			d.Set("updated_at", newTimestamp)
+		}
+
+		// Should NOT have cleared the ID
+		if d.Id() == "" {
+			t.Error("Expected ID to be preserved when destroy_on_drift=false, but it was cleared")
+		}
+
+		// Should have updated the timestamp
+		if updatedAt := d.Get("updated_at").(string); updatedAt != newTimestamp {
+			t.Errorf("Expected timestamp to be updated to %s, got %s", newTimestamp, updatedAt)
+		}
+	})
+
+	t.Run("default destroy_on_drift is true", func(t *testing.T) {
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":      "test-repo",
+			"secret_name":     "test-secret", 
+			"plaintext_value": "test-value",
+			// destroy_on_drift not set, should default to true
+		})
+
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if !destroyOnDrift {
+			t.Error("Expected destroy_on_drift to default to true")
+		}
+	})
+
+	t.Run("no drift when timestamps match", func(t *testing.T) {
+		timestamp := "2023-01-01T00:00:00Z"
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":       "test-repo",
+			"secret_name":      "test-secret", 
+			"plaintext_value":  "test-value",
+			"destroy_on_drift": true,
+			"updated_at":       timestamp,
+		})
+		d.SetId("test-secret")
+
+		// Simulate same timestamp (no external change)
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != timestamp {
+			d.SetId("") // This should NOT happen
+		}
+
+		// Should NOT have cleared the ID
+		if d.Id() == "" {
+			t.Error("Expected ID to be preserved when no drift detected, but it was cleared")
+		}
+	})
+
+	t.Run("destroy_on_drift field properties", func(t *testing.T) {
+		resource := resourceGithubActionsSecret()
+		driftField := resource.Schema["destroy_on_drift"]
+
+		// Should be optional
+		if driftField.Required {
+			t.Error("Expected destroy_on_drift to be optional, but it's required")
+		}
+
+		if !driftField.Optional {
+			t.Error("Expected destroy_on_drift to be optional")
+		}
+
+		// Should be boolean type
+		if driftField.Type.String() != "TypeBool" {
+			t.Errorf("Expected destroy_on_drift to be TypeBool, got %s", driftField.Type.String())
+		}
+
+		// Should have default value of true
+		if driftField.Default != true {
+			t.Errorf("Expected destroy_on_drift default to be true, got %v", driftField.Default)
+		}
+
+		// Should have description
+		if driftField.Description == "" {
+			t.Error("Expected destroy_on_drift to have a description")
+		}
+	})
+}
+
+// Test demonstrating the solution to GitHub issue #964
+func TestGithubActionsSecretIssue964Solution(t *testing.T) {
+	t.Run("solve issue 964 - prevent recreation when GUI changes secret", func(t *testing.T) {
+		// This test demonstrates the fix for:
+		// https://github.com/integrations/terraform-provider-github/issues/964
+		
+		// Scenario: User creates secret with Terraform, then updates value via GitHub GUI
+		// Expected: With destroy_on_drift=false, Terraform should not recreate the secret
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":       "my-repo",
+			"secret_name":      "WORKFLOW_PAT", 
+			"plaintext_value":  "CHANGE_ME", // Initial placeholder value
+			"destroy_on_drift": false,        // KEY FIX: Prevents recreation
+		})
+		d.SetId("WORKFLOW_PAT")
+		
+		// Set initial timestamp
+		originalTime := "2023-01-01T00:00:00Z"
+		d.Set("updated_at", originalTime)
+		
+		// Simulate: User changes secret value via GitHub GUI
+		// This changes the updated_at timestamp  
+		newTime := "2023-01-01T12:00:00Z" // Later timestamp = external change
+		
+		// Test the read function behavior - this is what happens during terraform plan/apply
+		destroyOnDrift := d.Get("destroy_on_drift").(bool) // false
+		if updatedAt, ok := d.GetOk("updated_at"); ok && !destroyOnDrift && updatedAt != newTime {
+			// With destroy_on_drift=false, we update timestamp but don't clear ID
+			d.Set("updated_at", newTime)
+		}
+		
+		// RESULT: Secret should NOT be marked for recreation
+		if d.Id() == "" {
+			t.Error("ISSUE #964 NOT FIXED: Secret was marked for recreation despite destroy_on_drift=false")
+		}
+		
+		// RESULT: Timestamp should be updated to acknowledge the change
+		if d.Get("updated_at").(string) != newTime {
+			t.Error("Expected timestamp to be updated to acknowledge external change")
+		}
+		
+		t.Logf("SUCCESS: Issue #964 solved - secret with destroy_on_drift=false does not get recreated on external changes")
+	})
 }