fix: Add destroy-on-drift property to the GitHub Action Secret resource schema (#2832)

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

* Add schema migration

* Add docs for destroy_on_drift

* lint fixes

* Terraform requires update on calculated fields so implented update and pointed at the create func

* Fixes merge drift issues with both org secrets and actions secrets

Author: nickfloyd

github/migrate_github_actions_secret.go

@@ -0,0 +1,36 @@
+package github
+
+import (
+	"fmt"
+	"log"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+)
+
+func resourceGithubActionsSecretMigrateState(v int, is *terraform.InstanceState, meta interface{}) (*terraform.InstanceState, error) {
+	switch v {
+	case 0:
+		log.Printf("[INFO] Found GitHub Actions Secret State v0; migrating to v1")
+		return migrateGithubActionsSecretStateV0toV1(is)
+	default:
+		return is, fmt.Errorf("unexpected schema version: %d", v)
+	}
+}
+
+func migrateGithubActionsSecretStateV0toV1(is *terraform.InstanceState) (*terraform.InstanceState, error) {
+	if is.Empty() {
+		log.Printf("[DEBUG] Empty InstanceState; nothing to migrate.")
+		return is, nil
+	}
+
+	log.Printf("[DEBUG] GitHub Actions Secret Attributes before migration: %#v", is.Attributes)
+
+	// Add the destroy_on_drift field with default value true if it doesn't exist
+	if _, ok := is.Attributes["destroy_on_drift"]; !ok {
+		is.Attributes["destroy_on_drift"] = "true"
+	}
+
+	log.Printf("[DEBUG] GitHub Actions Secret Attributes after State Migration: %#v", is.Attributes)
+
+	return is, nil
+}

github/migrate_github_actions_secret_test.go

@@ -0,0 +1,69 @@
+package github
+
+import (
+	"reflect"
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/terraform"
+)
+
+func TestMigrateGithubActionsSecretStateV0toV1(t *testing.T) {
+	// Secret without destroy_on_drift should get default value
+	oldAttributes := map[string]string{
+		"id":              "test-secret",
+		"repository":      "test-repo",
+		"secret_name":     "test-secret",
+		"created_at":      "2023-01-01T00:00:00Z",
+		"updated_at":      "2023-01-01T00:00:00Z",
+		"plaintext_value": "secret-value",
+	}
+
+	newState, err := migrateGithubActionsSecretStateV0toV1(&terraform.InstanceState{
+		ID:         "test-secret",
+		Attributes: oldAttributes,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expectedAttributes := map[string]string{
+		"id":               "test-secret",
+		"repository":       "test-repo",
+		"secret_name":      "test-secret",
+		"created_at":       "2023-01-01T00:00:00Z",
+		"updated_at":       "2023-01-01T00:00:00Z",
+		"plaintext_value":  "secret-value",
+		"destroy_on_drift": "true",
+	}
+	if !reflect.DeepEqual(newState.Attributes, expectedAttributes) {
+		t.Fatalf("Expected attributes:\n%#v\n\nGiven:\n%#v\n",
+			expectedAttributes, newState.Attributes)
+	}
+
+	// Secret with existing destroy_on_drift should be preserved
+	oldAttributesWithDrift := map[string]string{
+		"id":               "test-secret",
+		"repository":       "test-repo",
+		"secret_name":      "test-secret",
+		"destroy_on_drift": "false",
+	}
+
+	newState2, err := migrateGithubActionsSecretStateV0toV1(&terraform.InstanceState{
+		ID:         "test-secret",
+		Attributes: oldAttributesWithDrift,
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+
+	expectedAttributesWithDrift := map[string]string{
+		"id":               "test-secret",
+		"repository":       "test-repo",
+		"secret_name":      "test-secret",
+		"destroy_on_drift": "false",
+	}
+	if !reflect.DeepEqual(newState2.Attributes, expectedAttributesWithDrift) {
+		t.Fatalf("Expected attributes:\n%#v\n\nGiven:\n%#v\n",
+			expectedAttributesWithDrift, newState2.Attributes)
+	}
+}

github/resource_github_actions_organization_secret.go

@@ -228,10 +228,11 @@ func resourceGithubActionsOrganizationSecretRead(d *schema.ResourceData, meta in
 	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 !ok {
-		if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
-			return err
-		}
+	}
+
+	// Always update the timestamp to prevent repeated drift detection
+	if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
+		return err
 	}
 
 	return nil

github/resource_github_actions_organization_secret_drift_test.go

@@ -0,0 +1,86 @@
+package github
+
+import (
+	"testing"
+
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
+)
+
+// Test for the organization secret drift detection fix
+func TestGithubActionsOrganizationSecretDriftDetectionFix(t *testing.T) {
+	t.Run("always updates timestamp regardless of drift detection", func(t *testing.T) {
+		// This test verifies the fix for the issue where updated_at was not
+		// being set when drift was detected, causing repeated drift detection
+
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsOrganizationSecret().Schema, map[string]interface{}{
+			"secret_name":      "test-secret",
+			"plaintext_value":  "test-value",
+			"visibility":       "private",
+			"destroy_on_drift": true,
+			"updated_at":       "2023-01-01T00:00:00Z", // Old timestamp
+		})
+		d.SetId("test-secret")
+
+		// Simulate the updated_at logic from the read function
+		// This is what the actual GitHub API would return (newer timestamp)
+		newTimestamp := "2023-01-01T12:00:00Z"
+
+		// Simulate the drift detection logic from resourceGithubActionsOrganizationSecretRead
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != newTimestamp {
+			// This would log the drift and clear the ID
+			d.SetId("")
+		}
+
+		// This is the key fix - always update the timestamp
+		err := d.Set("updated_at", newTimestamp)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Verify that the timestamp was updated even though drift was detected
+		if d.Get("updated_at").(string) != newTimestamp {
+			t.Error("Expected updated_at to be set to new timestamp after drift detection")
+		}
+
+		// Verify that the ID was cleared due to drift detection
+		if d.Id() != "" {
+			t.Error("Expected ID to be cleared due to drift detection with destroy_on_drift=true")
+		}
+	})
+
+	t.Run("does not clear ID when destroy_on_drift is false", func(t *testing.T) {
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsOrganizationSecret().Schema, map[string]interface{}{
+			"secret_name":      "test-secret",
+			"plaintext_value":  "test-value",
+			"visibility":       "private",
+			"destroy_on_drift": false,
+			"updated_at":       "2023-01-01T00:00:00Z", // Old timestamp
+		})
+		d.SetId("test-secret")
+
+		newTimestamp := "2023-01-01T12:00:00Z"
+
+		// Simulate the drift detection logic
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != newTimestamp {
+			d.SetId("")
+		}
+
+		// Always update the timestamp
+		err := d.Set("updated_at", newTimestamp)
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		// Verify that the ID was NOT cleared when destroy_on_drift=false
+		if d.Id() != "test-secret" {
+			t.Error("Expected ID to remain when destroy_on_drift=false")
+		}
+
+		// Verify that the timestamp was still updated
+		if d.Get("updated_at").(string) != newTimestamp {
+			t.Error("Expected updated_at to be updated even when destroy_on_drift=false")
+		}
+	})
+}

github/resource_github_actions_secret.go

@@ -17,11 +17,17 @@ func resourceGithubActionsSecret() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceGithubActionsSecretCreateOrUpdate,
 		Read:   resourceGithubActionsSecretRead,
+		Update: resourceGithubActionsSecretCreateOrUpdate,
 		Delete: resourceGithubActionsSecretDelete,
 		Importer: &schema.ResourceImporter{
 			State: resourceGithubActionsSecretImport,
 		},
 
+		// Schema migration added to handle the addition of destroy_on_drift field
+		// Resources created before this field was added need it populated with default value
+		SchemaVersion: 1,
+		MigrateState:  resourceGithubActionsSecretMigrateState,
+
 		Schema: map[string]*schema.Schema{
 			"repository": {
 				Type:        schema.TypeString,
@@ -62,6 +68,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,13 +167,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 !ok {
-		if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
-			return err
-		}
+	}
+
+	// Always update the timestamp to prevent repeated drift detection
+	if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
+		return err
 	}
 
 	return nil