Update Step* Types to take in config data as opposed to reading from state

[nywilken]

Configuration data provided to builders, provisioners, and post-processors is often stored in a state bag state.Put("config", &p.config) in order to share data between steps needed to compute the builder/provisioner/post-processor run. While this is a convenient way of sharing data across steps it is an anti-pattern as most steps only ever need a subset of the configuration data, and because getting the config data from state state.Get("config").(*Config) requires a bit type casting.

In an effort to help mitigate potential issues with sharing configuration data between steps we would like to update Step types for builders/provisioners/post-processors so that they have fields for the config data they need, as opposed to relying on config being saved in a shared state bag.

This issue proposes the following changes to any builder/provisioner/post-processor that is currently relying on state.Get("config").(*Config) for obtaining read-only config data.

1. Identify the configuration data used by the step's Run function and add it as a field to the Step type.
2. Remove the call to state.get("config").(*Config) from the step's Run function; replacing the data with the new data stored in the Step type.
3. Update the caller of the step so that it passes in the new config data
4. Drop the line that puts the config into the state bag state.Put("config", &b.config) when all step types have been updated.

Example

//post-processors/vagrant-cloud/post-processor.go
...
state.Put("config", &p.config)
...
    // Build the steps                                                                                                                        
    steps := []multistep.Step{}                                                                                                               
    if p.config.BoxDownloadUrl == "" {                                                                                                        
      steps = []multistep.Step{                                                                                                               
        new(stepVerifyBox),                                                                                                                   
        new(stepCreateVersion),                                                                                                               
        new(stepCreateProvider),                                                                                                              
        new(stepPrepareUpload),                                                                                                               
        new(stepUpload),                                                                                                                      
-       new(stepReleaseVersion),                                                                                                           
+       &stepReleaseVersion{NoRelease: p.config.NoRelease},   
      }                                                                                                                                       
    } else {                                                                                                                                  
      steps = []multistep.Step{                                                                                                               
        new(stepVerifyBox),                                                                                                                   
        new(stepCreateVersion),                                                                                                               
        new(stepCreateProvider),   
       new(stepReleaseVersion),                                                                                                      
      }                                                                                                                                       
    }  


//post-processors/vagrant-cloud/step_release_version.go
type stepReleaseVersion struct {
+  NoRelease bool
}

func (s *stepReleaseVersion) Run(ctx context.Context, state multistep.StateBag) multistep.StepAction {
	client := state.Get("client").(*VagrantCloudClient)
	ui := state.Get("ui").(packer.Ui)
	box := state.Get("box").(*Box)
	version := state.Get("version").(*Version)
-	config := state.Get("config").(*Config)

	ui.Say(fmt.Sprintf("Releasing version: %s", version.Version))

-	if config.NoRelease {
+       if s.NoRelease
		ui.Message("Not releasing version due to configuration")
		return multistep.ActionContinue
	}
...

Becomes

Please note to help with reviewing these changes it is advised to open one pull-request per updated step type.

[SwampDragons]

This is probably going to be a slow and tedious implementation. Tn an ideal world, I'd like to see us slowly cleaning up these antipatterns over time as we make other improvements to a given step.

[teddylear]

I can try to take a look into this.

[teddylear]

Is this a problem anymore? I'm looking at file builder and it looks like it has it's own config that is being passed in. So is this still an issue?

[jagtstronaut]

https://github.com/search?q=repo%3Ahashicorp%2Fpacker%20state.get(%22config%22).(*Config)&type=code

Yeah I would close this. Looks like no longer an issue.

[nywilken]

Thanks for following up with issue. Now that all plugins have been removed from Packer I think this is safe to close. External plugin still need this work but that work is moving slowly during refactors.