[WIP] skip_register_ami option

[jeremy-asher]

I thought I'd put this up here as an idea for feedback before I do any more work.

It would be nice to avoid doing any AMI build steps in workflows where the AMI is not the true product and Packer is being used as a convenient mechanism to create a temporary AWS instance. I presume anyone using the artifice post processor would want this toggle.

Let me know if this makes any sense from a high level or implementation standpoint.

[pvanbuijtene]

We first test our images and destroy unused AMI's before creating the final, so you're not the only one interested in this feature ;)

[rickard-von-essen]

This makes sens to me.

[jeremy-asher]

Okay, I'll clean this up and add docs and param checks.

[mwhooker]

@jen20 can you mention this PR when you put your ebs builder up?

[mwhooker]

maybe we can replace this with

if c.AMISkipRegister {
    return
}
if c.AMIName == "" {
...

?

[mwhooker]

in https://github.com/mitchellh/packer/blob/master/builder/amazon/ebs/builder.go#L96-L103 it looks like we make use of the AMI name and some other config.

I wonder if we should either not run that step if we're skipping registration, or add another flag like ForceDeregister (eg SkipRegister) to make it a no op.

I generally like letting the steps decide if they're going to run or not, but we're already picking and choosing which steps we run based on config in this PR.

[jeremy-asher]

Sorry for the lag in following up with this. I think the rest of the validations in the prepare function can currently be skipped. I put a temporary note below there in the commit that I have to do additional validation that certain other params aren't supplied that don't have any effect with skip_register_ami, though.

Looks like those line references changed since you referenced them, but I presume you mean StepPreValidate and StepSourceAMIInfo. I suppose I got lucky that the PreValidate does still work, but it should be skipped. I don't see any reference to the AMI name in StepSourceAMIInfo. Did you mean a different step?

It looks like StepPreValidate already has ForceDeregister passed in which triggers an informational message. I'm not sure if that is a convention, but I could wrap the whole step like the others or pass a flag in. I don't mind either way, but for the larger blocks it seemed to make more sense to do it outside.

[jeremy-asher]

Also, before I start adding some validations that unused options aren't supplied, would it make sense to modify the signature of Prepare to return warnings as well as errors? I don't like the idea of crashing the run if unused options are supplied, but that seems to be the style for the Prepare functions.

[mwhooker]

the prepare functions are run before any work is done, so erroring is the correct behavior

let's just return early, here. I don't care so much about users having extraneous config

[mwhooker]

I think I was referring to &awscommon.StepPreValidate{. I would add another struct element to it, SkipRegister, and inside the step, check if that flag is set and return early if it is

[jeremy-asher]

Rebased to fix conflicts.

[jeremy-asher]

@mwhooker I have to do some more updates, but I could use your response to my earlier questions to close this out.

[mwhooker]

Don't think we need to do this

[mwhooker]

Added a couple of comments.

I think having so many steps being added inside of an if statement makes the code harder to read. Perhaps instead it could look something like:

steps := []multistep.Step{
...
}

amiCreationSteps := []multistep.Step{
... steps where we create the AMI
}

if ! c.AMISkipRegister {
    steps = append(steps, amiCreationSteps)
}

[jeremy-asher]

Thanks! I'll update this and make those changes.

[jeremy-asher]

I'm replacing this with #4681.