Merge branch 'main' into feature/allow-change-default-branch

Author: nickfloyd

.github/workflows/release.yml

@@ -14,7 +14,63 @@ permissions:
   contents: write
 
 jobs:
+  pre-release-tests:
+    name: Run tests before release
+    runs-on: ubuntu-latest
+    env:
+      test_stacks_directory: test_tf_stacks # root directory for test stacks
+      pre_release_tests: provider_only # directory name for pre-release tests
+    permissions:
+      contents: read
+      
+    steps:
+      - name: Checkout
+        uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0
+
+      - name: Setup Go
+        uses: actions/setup-go@44694675825211faa026b3c33043df3e48a5fa00 # v6.0.0
+        with:
+          go-version-file: go.mod
+          cache: true
+
+      - name: Build provider
+        run: go build -o terraform-provider-github
+
+      - name: Setup dev overrides
+        run: |
+          ROOT_DIR=$(pwd)
+          cat > ~/.terraformrc << EOF
+          provider_installation {
+            dev_overrides {
+              "integrations/github" = "${ROOT_DIR}"
+            }
+            direct {}
+          }
+          EOF
+
+      - name: Verify dev overrides setup
+        run: cat ~/.terraformrc
+
+      - name: Setup Terraform
+        uses: hashicorp/setup-terraform@b9cd54a3c349d3f38e8881555d616ced269862dd # v3.1.2
+        with:
+          terraform_version: 1.x
+
+      - name: Check Terraform version
+        run: terraform version
+
+      - name: Terraform init
+        continue-on-error: true # continue even if init fails
+        run: terraform -chdir=./${{ env.test_stacks_directory }}/${{ env.pre_release_tests }} init
+
+      - name: Terraform validate
+        run: terraform -chdir=./${{ env.test_stacks_directory }}/${{ env.pre_release_tests }} validate
+
+      - name: Clean up
+        run: rm -f ~/.terraformrc terraform-provider-github
+
   goreleaser:
+    needs: [ pre-release-tests ] # runs only if pre-release tests pass
     runs-on: ubuntu-latest
     steps:
       - uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0

github/resource_github_actions_organization_secret.go

@@ -17,7 +17,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 any) ([]*schema.ResourceData, error) {
@@ -61,8 +60,8 @@ func resourceGithubActionsOrganizationSecret() *schema.Resource {
 			"visibility": {
 				Type:             schema.TypeString,
 				Required:         true,
-				ValidateDiagFunc: validateValueFunc([]string{"all", "private", "selected"}),
 				ForceNew:         true,
+				ValidateDiagFunc: validateValueFunc([]string{"all", "private", "selected"}),
 				Description:      "Configures the access that repositories have to the organization secret. Must be one of 'all', 'private', or 'selected'. 'selected_repository_ids' is required if set to 'selected'.",
 			},
 			"selected_repository_ids": {
@@ -72,6 +71,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": {
@@ -85,9 +85,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.",
 			},
 		},
 	}
@@ -171,12 +173,6 @@ func resourceGithubActionsOrganizationSecretRead(d *schema.ResourceData, meta an
 		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
 	}
@@ -222,14 +218,35 @@ func resourceGithubActionsOrganizationSecretRead(d *schema.ResourceData, meta an
 	// 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]any{
+			"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]any{
+			"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]any{
+			"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

@@ -18,7 +18,6 @@ func resourceGithubActionsSecret() *schema.Resource {
 	return &schema.Resource{
 		Create: resourceGithubActionsSecretCreateOrUpdate,
 		Read:   resourceGithubActionsSecretRead,
-		Update: resourceGithubActionsSecretCreateOrUpdate,
 		Delete: resourceGithubActionsSecretDelete,
 		Importer: &schema.ResourceImporter{
 			State: resourceGithubActionsSecretImport,
@@ -73,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.",
 			},
 		},
@@ -144,12 +144,6 @@ func resourceGithubActionsSecretRead(d *schema.ResourceData, meta any) error {
 		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
 	}
@@ -165,17 +159,36 @@ func resourceGithubActionsSecretRead(d *schema.ResourceData, meta any) error {
 	// 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
 	}