-
Notifications
You must be signed in to change notification settings - Fork 9.9k
Pull init
command's Run method logic into separate method in new file, enable accessing experimental version of init logic via experiments and flags
#37327
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
base: main
Are you sure you want to change the base?
Conversation
…ration for adding experimental fork.
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.
init
command's Run method logic into separate method in new fileinit
command's Run method logic into separate method in new file, enable accessing experimental version of init logic via experiments and flags
👋🏻 @radeksimko - I figured that it would make sense to include the work to gate the experimental init logic behind a CLI flag & experimental status in this PR. I haven't included any automated tests for this work, but I've manually tested that the panic is reached only when experiments are added and the |
Also, it could be that we leave the original logic in the Run method and have the experimental logic in a separate file later, and then the experimental code is brought into the Run method? That would enable a single diff when the experimental changes are made non-experimental and would also preserve the existing code (idk if git blame would be easier to navigate if we kept it in its original place?) |
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.
Shall we also introduce an equivalent environment variable, to set us up for easier experimenting in various HCP and CI environments?
@@ -107,6 +112,9 @@ func ParseInit(args []string) (*Init, tfdiags.Diagnostics) { | |||
cmdFlags.Var(&init.BackendConfig, "backend-config", "") | |||
cmdFlags.Var(&init.PluginPath, "plugin-dir", "plugin directory") | |||
|
|||
// Used for enabling experimental code that's invoked before configuration is parsed. | |||
cmdFlags.BoolVar(&init.EnablePssExperiment, "enable-pss", false, "Enable the PSS experiment") |
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'm not too fussy about what we call it internally but I'm thinking the public-facing flag could be a bit clearer and longer, e.g. enable-pluggable-state-store-experiment
?
I know, very (!) mouthful 😅 but the benefit is that it exactly makes people think twice about why they are enabling it and make it very obvious this is not a regular flag.
// > The flag -enable-pss is passed to the init command. | ||
if c.Meta.AllowExperimentalFeatures && initArgs.EnablePssExperiment { | ||
// TODO(SarahFrench/radeksimko): Remove forked init logic once feature is no longer experimental | ||
panic("pss: experimental init code hasn't been added yet") |
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'm fine with the panic here but I think we should make the message just tiny bit more user-friendly since we are exposing it to the user now.
For example, how about This experiment is not available yet
?
Theoretically we could make it into a nice error but again - I'm fine with panic. 😄
This PR doesn't change the logic of the Run method on the init command, but it refactors code so that logic is in a separate file.
This is preparation for making non-trivial changes to the init command's Run method for the PSS project. Our intention is to end up with the Run command looking like this:
There would be the original version of the run logic in
internal/command/init_run.go
and in future we'd add a newer version of the logic in a yet-to-be-made file. We can then iterate on the newer version of the logic onmain
branch ok, and when PSS stops being experimental we would update the contents ofinternal/command/init_run.go
.Target Release
1.14.x
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