Reconcile the encrypted_value, plaintext_value in the read to unknown went dates don't match

Author: nickfloyd

github/resource_github_actions_organization_secret.go

@@ -172,12 +172,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
 	}
@@ -223,14 +217,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,114 @@ 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 requires ForceNew due to Update function", func(t *testing.T) {
+		// Test that destroy_on_drift field has ForceNew: true 
+		// This is required because we have an Update function defined  
+		schema := resourceGithubActionsOrganizationSecret().Schema["destroy_on_drift"]
+		if !schema.ForceNew {
+			t.Error("destroy_on_drift field must have ForceNew: true when Update function is defined, to avoid 'Provider produced inconsistent result' errors")
+		}
+
+		// Test that it defaults to true
+		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

@@ -143,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
 	}
@@ -164,14 +158,35 @@ 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("")
+		
+		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_secret_test.go

@@ -394,37 +394,87 @@ func TestGithubActionsSecretDriftDetection(t *testing.T) {
 		}
 	})
 
-	t.Run("destroyOnDrift false updates timestamp without recreation", func(t *testing.T) {
+	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, resourceGithubActionsSecret().Schema, map[string]interface{}{
 			"repository":       "test-repo", 
 			"secret_name":      "test-secret",
-			"plaintext_value":  "test-value",
+			"plaintext_value":  "original-value",
+			"encrypted_value":  "original-encrypted",
 			"destroy_on_drift": false,
 			"updated_at":       originalTimestamp,
 		})
 		d.SetId("test-secret")
 
-		// Test the drift detection logic when destroy_on_drift is false
+		// Simulate 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
+		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
+		// 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, resourceGithubActionsSecret().Schema, map[string]interface{}{
+			"repository":       "test-repo", 
+			"secret_name":      "test-secret",
+			"plaintext_value":  "original-value",
+			"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("default destroy_on_drift is true", func(t *testing.T) {
 		d := schema.TestResourceDataRaw(t, resourceGithubActionsSecret().Schema, map[string]interface{}{
 			"repository":      "test-repo",
@@ -463,6 +513,20 @@ func TestGithubActionsSecretDriftDetection(t *testing.T) {
 		}
 	})
 
+	t.Run("destroy_on_drift requires ForceNew due to Update function", func(t *testing.T) {
+		// Test that destroy_on_drift field has ForceNew: true 
+		// This is required because we have an Update function defined
+		schema := resourceGithubActionsSecret().Schema["destroy_on_drift"]
+		if !schema.ForceNew {
+			t.Error("destroy_on_drift field must have ForceNew: true when Update function is defined, to avoid 'Provider produced inconsistent result' errors")
+		}
+
+		// Test that it defaults to true
+		if schema.Default != true {
+			t.Error("destroy_on_drift should default to true for backward compatibility")
+		}
+	})
+
 	t.Run("destroy_on_drift field properties", func(t *testing.T) {
 		resource := resourceGithubActionsSecret()
 		driftField := resource.Schema["destroy_on_drift"]