fix: Correct secret drift implementation

[stevehipwell]

Resolves #3049

---

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]

👋 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]

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.

[stevehipwell]

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).

[stevehipwell]

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]

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]
  }
}

1. User runs terraform apply
2. Other user updates the secret via UI
3. 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.

[deiga]

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

[stevehipwell]

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.

[stevehipwell]

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

[deiga]

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

Could you mark this as deprecated?

[stevehipwell]

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]

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
		}
)

[stevehipwell]

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]

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

[stevehipwell]

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.