-
Notifications
You must be signed in to change notification settings - Fork 10.1k
Update data stored in plan files to enable using PSS with saved plans #37246
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
…ich is used when handling plan files
… plan file. Add some test coverage.
… PSS, if present
7b80adb
to
a2acc33
Compare
internal/plans/plan.go
Outdated
// TODO: Is this needed? Are there issues if the same provider is used for PSS and resource management also? | ||
if p.StateStore.Provider != nil { | ||
m[p.StateStore.Provider.Source.String()] = addrs.AbsProviderConfig{ | ||
Module: addrs.RootModule, // state_store block is only in Root | ||
Provider: *p.StateStore.Provider.Source, | ||
// No aliases used for PSS provider. | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This might benefit from some discussion/pair programming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - we should discuss this more thoroughly and talk through the consequences of our design choices here.
if plan.Backend.Type == "" || plan.Backend.Config == nil { | ||
// Store details about accessing state | ||
backendInUse := plan.Backend.Type != "" && plan.Backend.Config != nil | ||
stateStoreInUse := plan.StateStore.Type != "" && plan.StateStore.Config != nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like the variable extraction here along with great names.
internal/plans/plan.go
Outdated
// TODO: Is this needed? Are there issues if the same provider is used for PSS and resource management also? | ||
if p.StateStore.Provider != nil { | ||
m[p.StateStore.Provider.Source.String()] = addrs.AbsProviderConfig{ | ||
Module: addrs.RootModule, // state_store block is only in Root | ||
Provider: *p.StateStore.Provider.Source, | ||
// No aliases used for PSS provider. | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree - we should discuss this more thoroughly and talk through the consequences of our design choices here.
We may re-add this when we implement PSS for use during apply commands with plan files
… used for PSS anymore.
When we discussed this we agreed that we'd remove that code for now, as this method is only used in one place and the previous line that checks the config for needed providers will pull in entries from required_providers. As PSS requires an entry in required_providers, the provider necessary for PSS will already be identified. We might re-assess this in future, but for now we opted to not include code that we didn't fully understand the ramifications of. |
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. |
Some prototyping/implementation for the PSS project that's only relevant to the apply command.
These changes enable saving PSS config in a plan, similar to how a planfile can store backend config and be used to configure a
backend.Backend
during an apply command that is supplied with a pre-existing plan file.Target Release
N/A
Rollback Plan
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
CHANGELOG entry