fix: Correct secret drift implementation #3069

stevehipwell · 2026-01-09T20:16:13Z

Resolves #3049
Resolves #2782
Resolves #2913
Resolves #2047
Resolves #2518

Closes #2462
Closes #2499

Before the change?

After the change?

Pull request checklist

Schema migrations have been created if needed (example)
Tests for the changes have been added (for bug fixes / features)
Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

Yes
No

github-actions · 2026-01-09T20:16:24Z

👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

nickfloyd

I really like this approach vs what we have. I just had some thoughts to consider.

nickfloyd · 2026-01-09T21:08:26Z

github/resource_github_actions_environment_secret.go

-	//
-	// To solve this, we must unset the values and allow Terraform to decide whether or
-	// not this resource should be modified or left as-is (ignore_changes).
-	if updatedAt, ok := d.GetOk("updated_at"); ok && updatedAt != secret.UpdatedAt.String() {


I belive this will be a breaking change due to ignore_changes - there will be forced recreations now. Not sure if we want a migration path for the users who rely on that or just mark it as a breaking change.

Sorry, I'm not sure I follow how the new code would be breaking? The comment here is incorrect as this will always force new (note this resource doesn't have the destroy_on_drift input to suppress this behaviour).

Just looking into this further and it looks like this resource was relying on a lifecycle block to ignore changes. I've updated the docs to show this correctly being implemented on the new remote_updated_at field. I wouldn't consider a change to how meta arguments work a breaking change.

nickfloyd · 2026-01-09T21:12:50Z

github/resource_github_actions_environment_secret.go

+		ReadContext:   resourceGithubActionsEnvironmentSecretRead,
+		DeleteContext: resourceGithubActionsEnvironmentSecretDelete,
+
+		CustomizeDiff: func(ctx context.Context, diff *schema.ResourceDiff, m any) error {


I really like this approach. Any concerns around external changes to the resource forcing the destruction and recreation of the secret?

For instance...
Let's say a user config has:

resource "github_actions_environment_secret" "api_key" { ... plaintext_value = "secret-value-123" lifecycle { ignore_changes = [plaintext_value] } }

User runs terraform apply

Other user updates the secret via UI

User runs terraform apply

The old way the apply would detect no changes. This new approach updated_at and remote_updated_at would be different and therefore force a recreation.

AFAIK setting ignore_changes = [plaintext_value, updated_at, remote_updated_at] should resolve that

IMHO this is a documentation issue, what @deiga added should be the correct solution. If it's not then we'd need to add in the explicit destroy_on_drift field that I'd planned on deprecating.

The correct setting is ignore_changes = [remote_updated_at] and there is now a test to validate this.

deiga · 2026-01-09T21:32:13Z

github/acc_test.go

+var testAccProviders map[string]*schema.Provider = map[string]*schema.Provider{
+	"github": Provider(),
+}


AFAIK we should be getting rid of using testAccProviders the recommended approach is to only use ProviderFactories.

Could you mark this as deprecated?

I've made the change to do this already (and stashed them), but I'm holding off adding them in to the PR until the actual changes have been reviewed.

deiga · 2026-01-09T21:51:04Z

github/resource_github_actions_environment_secret.go

+		ReadContext:   resourceGithubActionsEnvironmentSecretRead,
+		DeleteContext: resourceGithubActionsEnvironmentSecretDelete,
+
+		CustomizeDiff: func(ctx context.Context, diff *schema.ResourceDiff, m any) error {


suggestion: I think we should be able to forgo adding the remote_updated_at field as we could use customdiff.ForceNewIfChange (https://pkg.go.dev/github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff#ForceNewIfChange) here

Something along the lines of:

CustomizeDiff: customdiff.ForceNewIfChange( "updated_at", func(ctx context.Context, oldValue, newValue, meta any) bool { remoteUpdatedAt := newValue if len(remoteUpdatedAt) == 0 { return false } updatedAt := oldValue if updatedAt != remoteUpdatedAt { if len(updatedAt) != 0 { return true } } return false } )

Unfortunately you can't do this (I actually coded it up to make sure my mental model was correct). If we're doing drift detection we can't rely on a field updated in the read path as it will never have a change in the pan path. In this case update_at is only updated in read if it's not been able to be calculated as part of the create path, this contrasts with remote_updated_at which is always updated.

deiga · 2026-01-09T21:51:49Z

github/resource_github_actions_environment_secret.go

+		ReadContext:   resourceGithubActionsEnvironmentSecretRead,
+		DeleteContext: resourceGithubActionsEnvironmentSecretDelete,
+
+		CustomizeDiff: func(ctx context.Context, diff *schema.ResourceDiff, m any) error {


AFAIK setting ignore_changes = [plaintext_value, updated_at, remote_updated_at] should resolve that

stevehipwell · 2026-01-12T17:37:41Z

I've updated the implementation to not require a recreate now that the diff is being handled correctly. I've also updated the pattern to correctly handle repo identity and support repo renames.

deiga · 2026-01-17T20:46:16Z

There is an open PR for adding import of resourceGithubActionsEnvironmentSecret
https://github.com/integrations/terraform-provider-github/pull/2462/changes#diff-bd7e54bc1e5ce2c06ae7d38565efee3c760620c960497f9c1d6db2f230415cbc

should that be taken into account or closed?

deiga · 2026-01-17T20:47:36Z

Further prior art on this topic: #2499

deiga · 2026-01-18T17:25:16Z

This might resolve #2047

stevehipwell · 2026-01-19T12:43:32Z

Thanks for the context @deiga. PR #2462 can be closed by this PR; FYI it has the same special character escaping issues seen across all of the environment resources (& data sources). PR #2499 can also be closed by this PR as it looks like it's just a refactoring of the existing flawed pattern. This PR should fix #2047, that's one of the failure mode it's intended to resolve.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2026-01-30T17:53:22Z

@deiga FYI I've updated this PR to use your ID as computed pattern, as well as shared diff functions.

deiga

Some comments. Disregard if not applicable due to draft state

deiga · 2026-01-31T07:48:40Z

github/resource_github_actions_environment_secret.go

 	} else {
 		d.SetId(id)
 	}


Not your change, but I wonder if the else is superfluous here

deiga · 2026-01-31T09:43:04Z

github/resource_github_actions_environment_secret.go

+	} else {
+		d.SetId(id)
+	}


question: does this else increase clarity?

deiga · 2026-01-31T09:46:57Z

github/resource_github_actions_environment_secret.go

-		if err = d.Set("updated_at", secret.UpdatedAt.String()); err != nil {
+	// GitHub API does not return on update so we have to lookup the secret to get timestamps
+	if secret, _, err := client.Actions.GetEnvSecret(ctx, repoID, escapedEnvName, secretName); err == nil {
+		if err := d.Set("created_at", secret.CreatedAt.String()); err != nil {


question: Is there a case where setting created_at is necessary in Update? I'm thinking that it's only and import and create concern

deiga · 2026-01-31T10:04:19Z

github/resource_github_actions_environment_secret.go

+		if err := d.Set("remote_updated_at", secret.UpdatedAt.String()); err != nil {
+			return diag.FromErr(err)
+		}
+	} else {


question: so, if the API returns an error when fetching secret, we set timestamps to nil. Why not I'd to nil as well? Isn't the secret then not existing?

deiga · 2026-01-31T10:05:25Z

github/resource_github_actions_environment_secret.go

+	envName := d.Get("environment").(string)
+	secretName := d.Get("secret_name").(string)
+
+	log.Printf("[INFO] Deleting environment secret: %s", d.Id())


question: would you be willing to make the tflog refactoring in the same PR? They should be fairly safe changes

deiga · 2026-01-31T10:06:15Z

github/resource_github_actions_environment_secret.go

+
+	repoName, envNamePart, secretName, err := parseID3(d.Id())
+	if err != nil {
+		return nil, err


question: Should the error message include the import ID format, to make the UX nicer?

deiga · 2026-01-31T10:09:19Z

github/resource_github_actions_environment_secret_migration_test.go

@@ -0,0 +1,53 @@
+package github
+
+// TODO: Enable this test one there is a way to mock the metadata.


question: which metadata are you referring to here?

deiga · 2026-01-31T10:11:34Z

github/resource_github_actions_environment_variable.go

+	})
 	if err != nil {
-		var ghErr *github.ErrorResponse
-		if errors.As(err, &ghErr) && ghErr.Response.StatusCode == http.StatusConflict {


Love this! Implicit import can feel nice for the user but is a TF antipattern

stevehipwell added this to the v6.10.0 Release milestone Jan 9, 2026

stevehipwell requested a review from nickfloyd January 9, 2026 20:16

stevehipwell self-assigned this Jan 9, 2026

stevehipwell added the Type: Bug Something isn't working as documented label Jan 9, 2026

stevehipwell force-pushed the fix-secrets-vars branch from d8a4694 to 4a14a57 Compare January 9, 2026 20:20

nickfloyd reviewed Jan 9, 2026

View reviewed changes

deiga reviewed Jan 9, 2026

View reviewed changes

stevehipwell mentioned this pull request Jan 12, 2026

[BUG]: Provider produced inconsistent result after apply (v6.9.1) #3049

Closed

1 task

stevehipwell force-pushed the fix-secrets-vars branch 3 times, most recently from 299e8db to c1c5bc3 Compare January 12, 2026 17:37

stevehipwell mentioned this pull request Jan 13, 2026

[MAINT] Fix github_organization_ruleset and github_repository_ruleset with push target #2958

Open

5 tasks

stevehipwell force-pushed the fix-secrets-vars branch 3 times, most recently from d34ded0 to 12aef66 Compare January 13, 2026 11:49

nickfloyd previously approved these changes Jan 13, 2026

View reviewed changes

stevehipwell modified the milestones: v6.10.0 Release, v6.11.0 Release Jan 13, 2026

stevehipwell dismissed nickfloyd’s stale review via f6933d8 January 15, 2026 17:21

stevehipwell force-pushed the fix-secrets-vars branch 5 times, most recently from 5a5289a to edfa9c9 Compare January 15, 2026 17:49

stevehipwell force-pushed the fix-secrets-vars branch from edfa9c9 to 4d05762 Compare January 19, 2026 12:37

fix: Correct secret drift implementation

852c032

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell force-pushed the fix-secrets-vars branch from 4d05762 to 852c032 Compare January 30, 2026 17:51

deiga reviewed Jan 31, 2026

View reviewed changes

		@@ -0,0 +1,53 @@
		package github

		// TODO: Enable this test one there is a way to mock the metadata.

fix: Correct secret drift implementation #3069

Are you sure you want to change the base?

fix: Correct secret drift implementation #3069

Uh oh!

Conversation

stevehipwell commented Jan 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions bot commented Jan 9, 2026

Uh oh!

nickfloyd left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stevehipwell commented Jan 12, 2026

Uh oh!

deiga commented Jan 17, 2026

Uh oh!

deiga commented Jan 17, 2026

Uh oh!

deiga commented Jan 18, 2026

Uh oh!

stevehipwell commented Jan 19, 2026

Uh oh!

stevehipwell commented Jan 30, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

stevehipwell commented Jan 9, 2026 •

edited

Loading