Merge v6.7.5 fixes into v6.8.1 patch release

Author: nickfloyd

github/resource_github_actions_organization_secret.go

@@ -16,7 +16,6 @@ func resourceGithubActionsOrganizationSecret() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceGithubActionsOrganizationSecretCreateOrUpdate,
 		Read:   resourceGithubActionsOrganizationSecretRead,
-		Update: resourceGithubActionsOrganizationSecretCreateOrUpdate,
 		Delete: resourceGithubActionsOrganizationSecretDelete,
 		Importer: &schema.ResourceImporter{
 			State: func(d *schema.ResourceData, meta interface{}) ([]*schema.ResourceData, error) {
@@ -71,6 +70,7 @@ func resourceGithubActionsOrganizationSecret() *schema.Resource {
 				},
 				Set:         schema.HashInt,
 				Optional:    true,
+				ForceNew:    true,
 				Description: "An array of repository ids that can access the organization secret.",
 			},
 			"created_at": {
@@ -84,9 +84,11 @@ func resourceGithubActionsOrganizationSecret() *schema.Resource {
 				Description: "Date of 'actions_secret' update.",
 			},
 			"destroy_on_drift": {
-				Type:     schema.TypeBool,
-				Default:  true,
-				Optional: true,
+				Type:        schema.TypeBool,
+				Default:     true,
+				Optional:    true,
+				ForceNew:    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.",
 			},
 		},
 	}
@@ -169,12 +171,6 @@ func resourceGithubActionsOrganizationSecretRead(d *schema.ResourceData, meta in
 		return err
 	}
 
-	if err = d.Set("encrypted_value", d.Get("encrypted_value")); err != nil {
-		return err
-	}
-	if err = d.Set("plaintext_value", d.Get("plaintext_value")); err != nil {
-		return err
-	}
 	if err = d.Set("created_at", secret.CreatedAt.String()); err != nil {
 		return err
 	}
@@ -220,14 +216,35 @@ func resourceGithubActionsOrganizationSecretRead(d *schema.ResourceData, meta in
 	// timestamp we've persisted in the state. In that case, we can no longer
 	// trust that the value (which we don't see) is equal to what we've declared
 	// previously.
-	//
-	// 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.
 	destroyOnDrift := d.Get("destroy_on_drift").(bool)
-	if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != secret.UpdatedAt.String() {
+	storedUpdatedAt, hasStoredUpdatedAt := d.GetOk("updated_at")
+	
+	if hasStoredUpdatedAt && storedUpdatedAt != secret.UpdatedAt.String() {
 		log.Printf("[INFO] The secret %s has been externally updated in GitHub", d.Id())
-		d.SetId("")
+		
+		if destroyOnDrift {
+			// Original behavior: mark for recreation
+			d.SetId("")
+			return nil
+		} else {
+			// Alternative approach: set sensitive values to empty to trigger update plan
+			// This tells Terraform that the current state is unknown and needs reconciliation
+			if err = d.Set("encrypted_value", ""); err != nil {
+				return err
+			}
+			if err = d.Set("plaintext_value", ""); err != nil {
+				return err
+			}
+			log.Printf("[INFO] Detected drift but destroy_on_drift=false, clearing sensitive values to trigger update")
+		}
+	} else {
+		// No drift detected, preserve the configured values in state
+		if err = d.Set("encrypted_value", d.Get("encrypted_value")); err != nil {
+			return err
+		}
+		if err = d.Set("plaintext_value", d.Get("plaintext_value")); err != nil {
+			return err
+		}
 	}
 
 	// Always update the timestamp to prevent repeated drift detection

github/resource_github_actions_organization_secret_test.go

@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/resource"
+	"github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema"
 )
 
 func TestAccGithubActionsOrganizationSecret(t *testing.T) {
@@ -184,4 +185,108 @@ func TestAccGithubActionsOrganizationSecret(t *testing.T) {
 			testCase(t, organization)
 		})
 	})
+
+	// Unit tests for drift detection behavior
+	t.Run("destroyOnDrift false clears sensitive values instead of recreating", func(t *testing.T) {
+		originalTimestamp := "2023-01-01T00:00:00Z"
+		newTimestamp := "2023-01-02T00:00:00Z"
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsOrganizationSecret().Schema, map[string]interface{}{
+			"secret_name":      "test-secret",
+			"plaintext_value":  "original-value",
+			"encrypted_value":  "original-encrypted",
+			"visibility":       "private",
+			"destroy_on_drift": false,
+			"updated_at":       originalTimestamp,
+		})
+		d.SetId("test-secret")
+
+		// Simulate drift detection logic when destroy_on_drift is false
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		storedUpdatedAt, hasStoredUpdatedAt := d.GetOk("updated_at")
+		
+		if hasStoredUpdatedAt && storedUpdatedAt != newTimestamp {
+			if destroyOnDrift {
+				// Would clear ID for recreation
+				d.SetId("")
+			} else {
+				// Should clear sensitive values to trigger update
+				d.Set("encrypted_value", "")
+				d.Set("plaintext_value", "")
+			}
+			d.Set("updated_at", newTimestamp)
+		}
+
+		// Should NOT have cleared the ID when destroy_on_drift=false
+		if d.Id() == "" {
+			t.Error("Expected ID to be preserved when destroy_on_drift=false, but it was cleared")
+		}
+
+		// Should have cleared sensitive values to trigger update plan
+		if plaintextValue := d.Get("plaintext_value").(string); plaintextValue != "" {
+			t.Errorf("Expected plaintext_value to be cleared for update plan, got %s", plaintextValue)
+		}
+
+		if encryptedValue := d.Get("encrypted_value").(string); encryptedValue != "" {
+			t.Errorf("Expected encrypted_value to be cleared for update plan, got %s", encryptedValue)
+		}
+
+		// 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("destroyOnDrift true still recreates resource on drift", func(t *testing.T) {
+		originalTimestamp := "2023-01-01T00:00:00Z"
+		newTimestamp := "2023-01-02T00:00:00Z"
+		
+		d := schema.TestResourceDataRaw(t, resourceGithubActionsOrganizationSecret().Schema, map[string]interface{}{
+			"secret_name":      "test-secret",
+			"plaintext_value":  "original-value",
+			"visibility":       "private",
+			"destroy_on_drift": true,  // Explicitly set to true
+			"updated_at":       originalTimestamp,
+		})
+		d.SetId("test-secret")
+
+		// Simulate drift detection logic when destroy_on_drift is true
+		destroyOnDrift := d.Get("destroy_on_drift").(bool)
+		storedUpdatedAt, hasStoredUpdatedAt := d.GetOk("updated_at")
+		
+		if hasStoredUpdatedAt && storedUpdatedAt != newTimestamp {
+			if destroyOnDrift {
+				// Should clear ID for recreation (original behavior)
+				d.SetId("")
+				return // Exit early like the real function would
+			}
+		}
+
+		// Should have cleared the ID for recreation when destroy_on_drift=true  
+		if d.Id() != "" {
+			t.Error("Expected ID to be cleared for recreation when destroy_on_drift=true, but it was preserved")
+		}
+	})
+
+	t.Run("destroy_on_drift field defaults", func(t *testing.T) {
+		// Test that destroy_on_drift defaults to true for backward compatibility
+		schema := resourceGithubActionsOrganizationSecret().Schema["destroy_on_drift"]
+		if schema.Default != true {
+			t.Error("destroy_on_drift should default to true for backward compatibility")
+		}
+	})
+
+	t.Run("default destroy_on_drift is true", 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 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")
+		}
+	})
 }

github/resource_github_actions_secret.go

@@ -72,6 +72,7 @@ func resourceGithubActionsSecret() *schema.Resource {
 				Type:        schema.TypeBool,
 				Default:     true,
 				Optional:    true,
+				ForceNew:    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.",
 			},
 		},
@@ -142,12 +143,6 @@ func resourceGithubActionsSecretRead(d *schema.ResourceData, meta interface{}) e
 		return err
 	}
 
-	if err = d.Set("encrypted_value", d.Get("encrypted_value")); err != nil {
-		return err
-	}
-	if err = d.Set("plaintext_value", d.Get("plaintext_value")); err != nil {
-		return err
-	}
 	if err = d.Set("created_at", secret.CreatedAt.String()); err != nil {
 		return err
 	}
@@ -163,17 +158,36 @@ func resourceGithubActionsSecretRead(d *schema.ResourceData, meta interface{}) e
 	// timestamp we've persisted in the state. In that case, we can no longer
 	// trust that the value (which we don't see) is equal to what we've declared
 	// previously.
-	//
-	// 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.
 	destroyOnDrift := d.Get("destroy_on_drift").(bool)
-	if updatedAt, ok := d.GetOk("updated_at"); ok && destroyOnDrift && updatedAt != secret.UpdatedAt.String() {
+	storedUpdatedAt, hasStoredUpdatedAt := d.GetOk("updated_at")
+
+	if hasStoredUpdatedAt && storedUpdatedAt != secret.UpdatedAt.String() {
 		log.Printf("[INFO] The secret %s has been externally updated in GitHub", d.Id())
-		d.SetId("")
-	}
 
-	// Always update the timestamp to prevent repeated drift detection
+		if destroyOnDrift {
+			// Original behavior: mark for recreation
+			d.SetId("")
+			return nil
+		} else {
+			// Alternative approach: set sensitive values to empty to trigger update plan
+			// This tells Terraform that the current state is unknown and needs reconciliation
+			if err = d.Set("encrypted_value", ""); err != nil {
+				return err
+			}
+			if err = d.Set("plaintext_value", ""); err != nil {
+				return err
+			}
+			log.Printf("[INFO] Detected drift but destroy_on_drift=false, clearing sensitive values to trigger update")
+		}
+	} else {
+		// No drift detected, preserve the configured values in state
+		if err = d.Set("encrypted_value", d.Get("encrypted_value")); err != nil {
+			return err
+		}
+		if err = d.Set("plaintext_value", d.Get("plaintext_value")); 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
 	}