Skip to content

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

Open
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

SarahFrench
Copy link
Member

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:

// internal/command/init.go
func (c *InitCommand) run(args []string) int {
    if c.Meta.AllowExperimentalFeatures {
        // use experimental version of init logic
    } else {
        // use original version of init logic in internal/command/init_run.go
    }
}

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 on main branch ok, and when PSS stops being experimental we would update the contents of internal/command/init_run.go.

Target Release

1.14.x

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 Jul 15, 2025
@SarahFrench SarahFrench marked this pull request as ready for review July 15, 2025 15:37
@SarahFrench SarahFrench requested a review from a team as a code owner July 15, 2025 15:37
radeksimko
radeksimko previously approved these changes Jul 15, 2025
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.

:shipit:

@SarahFrench SarahFrench changed the title Pull init command's Run method logic into separate method in new file Pull init command's Run method logic into separate method in new file, enable accessing experimental version of init logic via experiments and flags Jul 18, 2025
@SarahFrench SarahFrench requested a review from radeksimko July 18, 2025 17:56
@SarahFrench
Copy link
Member Author

👋🏻 @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 -enable-pss flag is passed to the init command.

@SarahFrench
Copy link
Member Author

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

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.

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")
Copy link
Member

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")
Copy link
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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