Skip to content

Conversation

@SarahFrench
Copy link
Member

@SarahFrench SarahFrench commented Sep 17, 2025

We kinda need the ability to use TF_REATTACH_PROVIDERS during development of PSS to work out issues with the protocol, so I've implemented the ability to use it with PSS in this branch, specifically this commit e84e15b. We're probably going to allow users to use TF_REATTACH_PROVIDERS with PSS in the end, too.

When creating the backend state file, to record how PSS is being used, this method will be used to determine if the provider is reattached and therefore needs special handling.

This PR:

  • Adds test coverage for the preexisting parseReattachProviders function that parses TF_REATTACH_PROVIDERS and allows that ENV to be used.
  • Adds a new function that's inspired by the function above, that just checks if a given provider is being supplied via TF_REATTACH_PROVIDERS.
  • Moves reattach-related code into its own package

Target Release

N/A

Rollback Plan

  • If a change needs to be reverted, we will roll out an update to the code within 7 days.

Changes to Security Controls

Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.

CHANGELOG entry

  • This change is user-facing and I added a changelog entry.
  • This change is not user-facing.

@SarahFrench SarahFrench added the no-changelog-needed Add this to your PR if the change does not require a changelog entry label Sep 19, 2025
@SarahFrench SarahFrench force-pushed the pss/use-reattached-providers branch from fa160c5 to 0a4379e Compare September 19, 2025 10:30
@SarahFrench SarahFrench changed the title Exploring how to let TF_REATTACH_PROVIDERS be used with PSS First step to enable use of TF_REATTACH_PROVIDERS with PSS Sep 19, 2025
@SarahFrench SarahFrench marked this pull request as ready for review September 19, 2025 10:31
@SarahFrench SarahFrench requested a review from a team as a code owner September 19, 2025 10:31
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM aside from that one question

Comment on lines 20 to 21
func ParseReattachProviders() (map[addrs.Provider]*plugin.ReattachConfig, error) {
in := os.Getenv(TF_REATTACH_PROVIDERS)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't it be easier to test things and reuse the function if we continued passing the value in as an argument, rather than read the environment variable?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah sure, we can avoid use of t.Setenv in tests that way. My rationalisation was that moving the call to os.Getenv into this package meant that the "TF_REATTACH_PROVIDERS" string wasn't floating around the codebase, but I can address that by making calling code use the new constant.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've refactored the function signature to require the new parameters in 12f8da6

radeksimko
radeksimko previously approved these changes Sep 19, 2025
@SarahFrench SarahFrench enabled auto-merge (squash) September 19, 2025 11:38
@SarahFrench SarahFrench merged commit 0dfa115 into main Sep 19, 2025
7 checks passed
@SarahFrench SarahFrench deleted the pss/use-reattached-providers branch September 19, 2025 11:42
@github-actions
Copy link
Contributor

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Oct 20, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

no-changelog-needed Add this to your PR if the change does not require a changelog entry

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants