From ca197afe9b70398bc0e9ee6f27fd6e6cc6ef7f83 Mon Sep 17 00:00:00 2001 From: Lucas Bajolet Date: Mon, 5 Dec 2022 16:06:07 -0500 Subject: [PATCH] hcp: remove superfluous return value on GetBuilds This commit irons out one of the pain points of the HCP rework by introducing a HCPPublisher interface, implemented both by the JSON Core, and the HCL2 PackerConfig, which keeps a map of the build names used by Packer to the build names pushed on HCP. This in turn lets us go back to the old implementation of the GetBuilds function, which returns a list of (filtered) builds, and eventually an error if something went wrong while processing. --- CHANGELOG.md | 8 ++ command/build.go | 6 +- command/build_test.go | 131 ++++++++++++++++++ .../provisioners/provisioner-only-except.json | 37 +++++ .../provisioner-only-except.pkr.hcl | 31 +++++ command/validate.go | 2 +- hcl2template/common_test.go | 2 +- hcl2template/types.packer_config.go | 15 +- internal/hcp/registry/hcl.go | 19 ++- internal/hcp/registry/json.go | 20 ++- internal/hcp/registry/null_registry.go | 4 +- internal/hcp/registry/registry.go | 4 +- packer/core.go | 37 ++--- packer/run_interfaces.go | 2 +- 14 files changed, 263 insertions(+), 55 deletions(-) create mode 100644 command/test-fixtures/provisioners/provisioner-only-except.json create mode 100644 command/test-fixtures/provisioners/provisioner-only-except.pkr.hcl diff --git a/CHANGELOG.md b/CHANGELOG.md index f69148195..b5bc6a55b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,6 +11,14 @@ display the proper filename and line number where the unknown reference resides. [GH-12167](https://github.com/hashicorp/packer/pull/12167) +### NOTES: +* core: Users will see some changes in how names are displayed during a Packer + build for JSON templates. Previously only the builder type or the builder + name, if it was set, would be displayed. Now for named builders + (`"name":"mybuilder"`) the builder'ss type and name will be displayed (i.e + ".mybuilder". This does not impact the behavior of options such as + only or except, they will continue to work as they did before.) + ## 1.8.5 (December 12, 2022) ### NOTES: diff --git a/command/build.go b/command/build.go index 1cf9cdd61..66a2ec4c9 100644 --- a/command/build.go +++ b/command/build.go @@ -107,7 +107,7 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int }) } - builds, hcpMap, diags := packerStarter.GetBuilds(packer.GetBuildsOptions{ + builds, diags := packerStarter.GetBuilds(packer.GetBuildsOptions{ Only: cla.Only, Except: cla.Except, Debug: cla.Debug, @@ -219,7 +219,7 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int defer limitParallel.Release(1) - err := hcpRegistry.StartBuild(buildCtx, hcpMap[name]) + err := hcpRegistry.StartBuild(buildCtx, b) // Seems odd to require this error check here. Now that it is an error we can just exit with diag if err != nil { // If the build is already done, we skip without a warning @@ -249,7 +249,7 @@ func (c *BuildCommand) RunContext(buildCtx context.Context, cla *BuildArgs) int runArtifacts, hcperr := hcpRegistry.CompleteBuild( buildCtx, - hcpMap[name], + b, runArtifacts, err) if hcperr != nil { diff --git a/command/build_test.go b/command/build_test.go index a306fbde2..8aa30c720 100644 --- a/command/build_test.go +++ b/command/build_test.go @@ -1113,6 +1113,137 @@ func TestBuildCommand_ParseArgs(t *testing.T) { } } +// TestProvisionerOnlyExcept checks that only/except blocks in provisioners/post-processors behave as expected +func TestProvisionerAndPostProcessorOnlyExcept(t *testing.T) { + tests := []struct { + name string + args []string + expectedCode int + outputCheck func(string, string) error + }{ + { + "json - only named build", + []string{ + "-only", "packer", + testFixture("provisioners", "provisioner-only-except.json"), + }, + 0, + func(out, _ string) error { + if !strings.Contains(out, "packer provisioner packer and null") { + return fmt.Errorf("missing expected provisioner output") + } + + if !strings.Contains(out, "packer post-processor packer and null") { + return fmt.Errorf("missing expected post-processor output") + } + + if strings.Contains(out, "null post-processor") || strings.Contains(out, "null provisioner") { + return fmt.Errorf("found traces of unnamed provisioner/post-processor, should not") + } + + return nil + }, + }, + { + "json - only unnamed build", + []string{ + "-only", "null", + testFixture("provisioners", "provisioner-only-except.json"), + }, + 0, + func(out, _ string) error { + if !strings.Contains(out, "null provisioner null and null") { + return fmt.Errorf("missing expected provisioner output") + } + + if !strings.Contains(out, "null post-processor null and null") { + return fmt.Errorf("missing expected post-processor output") + } + + if strings.Contains(out, "packer post-processor") || strings.Contains(out, "packer provisioner") { + return fmt.Errorf("found traces of named provisioner/post-processor, should not") + } + + return nil + }, + }, + { + "hcl - only one source build", + []string{ + "-only", "null.packer", + testFixture("provisioners", "provisioner-only-except.pkr.hcl"), + }, + 0, + func(out, _ string) error { + if !strings.Contains(out, "packer provisioner packer and null") { + return fmt.Errorf("missing expected provisioner output") + } + + if !strings.Contains(out, "packer post-processor packer and null") { + return fmt.Errorf("missing expected post-processor output") + } + + if strings.Contains(out, "other post-processor") || strings.Contains(out, "other provisioner") { + return fmt.Errorf("found traces of other provisioner/post-processor, should not") + } + + return nil + }, + }, + { + "hcl - only other build", + []string{ + "-only", "null.other", + testFixture("provisioners", "provisioner-only-except.pkr.hcl"), + }, + 0, + func(out, _ string) error { + if !strings.Contains(out, "other provisioner other and null") { + return fmt.Errorf("missing expected provisioner output") + } + + if !strings.Contains(out, "other post-processor other and null") { + return fmt.Errorf("missing expected post-processor output") + } + + if strings.Contains(out, "packer post-processor") || strings.Contains(out, "packer provisioner") { + return fmt.Errorf("found traces of \"packer\" source provisioner/post-processor, should not") + } + + return nil + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + c := &BuildCommand{ + Meta: TestMetaFile(t), + } + + exitCode := c.Run(tt.args) + if exitCode != tt.expectedCode { + t.Errorf("process exit code mismatch: expected %d, got %d", + tt.expectedCode, + exitCode) + } + + out, stderr := GetStdoutAndErrFromTestMeta(t, c.Meta) + err := tt.outputCheck(out, stderr) + if err != nil { + if len(out) != 0 { + t.Logf("command stdout: %q", out) + } + + if len(stderr) != 0 { + t.Logf("command stderr: %q", stderr) + } + t.Error(err.Error()) + } + }) + } +} + // TestBuildCmd aims to test the build command, with output validation func TestBuildCmd(t *testing.T) { tests := []struct { diff --git a/command/test-fixtures/provisioners/provisioner-only-except.json b/command/test-fixtures/provisioners/provisioner-only-except.json new file mode 100644 index 000000000..6eee42387 --- /dev/null +++ b/command/test-fixtures/provisioners/provisioner-only-except.json @@ -0,0 +1,37 @@ +{ + "builders": [ + { + "type": "null", + "communicator": "none" + }, + { + "type": "null", + "name": "packer", + "communicator": "none" + } + ], + "provisioners": [ + { + "type": "shell-local", + "inline": ["echo packer provisioner {{build_name}} and {{build_type}}"], + "only": ["packer"] + }, + { + "type": "shell-local", + "inline": ["echo null provisioner {{build_name}} and {{build_type}}"], + "except": ["packer"] + } + ], + "post-processors": [ + { + "type": "shell-local", + "inline": ["echo packer post-processor {{build_name}} and {{build_type}}"], + "only": ["packer"] + }, + { + "type": "shell-local", + "inline": ["echo null post-processor {{build_name}} and {{build_type}}"], + "except": ["packer"] + } + ] +} diff --git a/command/test-fixtures/provisioners/provisioner-only-except.pkr.hcl b/command/test-fixtures/provisioners/provisioner-only-except.pkr.hcl new file mode 100644 index 000000000..811896538 --- /dev/null +++ b/command/test-fixtures/provisioners/provisioner-only-except.pkr.hcl @@ -0,0 +1,31 @@ +source "null" "packer" { + communicator = "none" +} + +source "null" "other" { + communicator = "none" +} + +build { + sources = ["sources.null.packer", "null.other"] + + provisioner "shell-local" { + inline = ["echo packer provisioner {{build_name}} and {{build_type}}"] + only = ["null.packer"] + } + + provisioner "shell-local" { + inline = ["echo other provisioner {{build_name}} and {{build_type}}"] + except = ["null.packer"] + } + + post-processor "shell-local" { + inline = ["echo packer post-processor {{build_name}} and {{build_type}}"] + only = ["null.packer"] + } + + post-processor "shell-local" { + inline = ["echo other post-processor {{build_name}} and {{build_type}}"] + except = ["null.packer"] + } +} diff --git a/command/validate.go b/command/validate.go index 657d71ebc..08eced959 100644 --- a/command/validate.go +++ b/command/validate.go @@ -70,7 +70,7 @@ func (c *ValidateCommand) RunContext(ctx context.Context, cla *ValidateArgs) int return ret } - _, _, diags = packerStarter.GetBuilds(packer.GetBuildsOptions{ + _, diags = packerStarter.GetBuilds(packer.GetBuildsOptions{ Only: cla.Only, Except: cla.Except, }) diff --git a/hcl2template/common_test.go b/hcl2template/common_test.go index d94cf945a..5b1c68fbd 100644 --- a/hcl2template/common_test.go +++ b/hcl2template/common_test.go @@ -106,7 +106,7 @@ func testParse(t *testing.T, tests []parseTest) { return } - gotBuilds, _, gotDiags := gotCfg.GetBuilds(packer.GetBuildsOptions{}) + gotBuilds, gotDiags := gotCfg.GetBuilds(packer.GetBuildsOptions{}) if tt.getBuildsWantDiags == (gotDiags == nil) { t.Fatalf("Parser.getBuilds() unexpected diagnostics. %s", gotDiags) } diff --git a/hcl2template/types.packer_config.go b/hcl2template/types.packer_config.go index 8325d0e90..ef67f33e7 100644 --- a/hcl2template/types.packer_config.go +++ b/hcl2template/types.packer_config.go @@ -564,20 +564,17 @@ func (cfg *PackerConfig) getCoreBuildPostProcessors(source SourceUseBlock, block // GetBuilds returns a list of packer Build based on the HCL2 parsed build // blocks. All Builders, Provisioners and Post Processors will be started and // configured. -func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Build, map[string]string, hcl.Diagnostics) { +func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Build, hcl.Diagnostics) { res := []packersdk.Build{} var diags hcl.Diagnostics possibleBuildNames := []string{} - // hcpTranslationMap maps the local name of a Corebuild to its HCP name - hcpTranslationMap := map[string]string{} - cfg.debug = opts.Debug cfg.force = opts.Force cfg.onError = opts.OnError if len(cfg.Builds) == 0 { - return res, hcpTranslationMap, append(diags, &hcl.Diagnostic{ + return res, append(diags, &hcl.Diagnostic{ Summary: "Missing build block", Detail: "A build block with one or more sources is required for executing a build.", Severity: hcl.DiagError, @@ -602,8 +599,6 @@ func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Bu Type: srcUsage.String(), } - hcpTranslationMap[pcb.Name()] = srcUsage.String() - pcb.SetDebug(cfg.debug) pcb.SetForce(cfg.force) pcb.SetOnError(cfg.onError) @@ -615,7 +610,7 @@ func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Bu if len(opts.Only) > 0 { onlyGlobs, diags := convertFilterOption(opts.Only, "only") if diags.HasErrors() { - return nil, nil, diags + return nil, diags } cfg.only = onlyGlobs include := false @@ -635,7 +630,7 @@ func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Bu if len(opts.Except) > 0 { exceptGlobs, diags := convertFilterOption(opts.Except, "except") if diags.HasErrors() { - return nil, nil, diags + return nil, diags } cfg.except = exceptGlobs exclude := false @@ -732,7 +727,7 @@ func (cfg *PackerConfig) GetBuilds(opts packer.GetBuildsOptions) ([]packersdk.Bu "These could also be matched with a glob pattern like: 'happycloud.*'", possibleBuildNames), }) } - return res, hcpTranslationMap, diags + return res, diags } var PackerConsoleHelp = strings.TrimSpace(` diff --git a/internal/hcp/registry/hcl.go b/internal/hcp/registry/hcl.go index ccbc1f577..baa2b5cb2 100644 --- a/internal/hcp/registry/hcl.go +++ b/internal/hcp/registry/hcl.go @@ -8,6 +8,7 @@ import ( "github.com/hashicorp/hcp-sdk-go/clients/cloud-packer-service/stable/2021-04-30/models" sdkpacker "github.com/hashicorp/packer-plugin-sdk/packer" "github.com/hashicorp/packer/hcl2template" + "github.com/hashicorp/packer/packer" "github.com/zclconf/go-cty/cty" "github.com/zclconf/go-cty/cty/gocty" ) @@ -45,18 +46,28 @@ func (h *HCLMetadataRegistry) PopulateIteration(ctx context.Context) error { } // StartBuild is invoked when one build for the configuration is starting to be processed -func (h *HCLMetadataRegistry) StartBuild(ctx context.Context, buildName string) error { - return h.bucket.startBuild(ctx, buildName) +func (h *HCLMetadataRegistry) StartBuild(ctx context.Context, build sdkpacker.Build) error { + name := build.Name() + cb, ok := build.(*packer.CoreBuild) + if ok { + name = cb.Type + } + return h.bucket.startBuild(ctx, name) } // CompleteBuild is invoked when one build for the configuration has finished func (h *HCLMetadataRegistry) CompleteBuild( ctx context.Context, - buildName string, + build sdkpacker.Build, artifacts []sdkpacker.Artifact, buildErr error, ) ([]sdkpacker.Artifact, error) { - return h.bucket.completeBuild(ctx, buildName, artifacts, buildErr) + name := build.Name() + cb, ok := build.(*packer.CoreBuild) + if ok { + name = cb.Type + } + return h.bucket.completeBuild(ctx, name, artifacts, buildErr) } func NewHCLMetadataRegistry(config *hcl2template.PackerConfig) (*HCLMetadataRegistry, hcl.Diagnostics) { diff --git a/internal/hcp/registry/json.go b/internal/hcp/registry/json.go index 20e1f808e..f15c79aed 100644 --- a/internal/hcp/registry/json.go +++ b/internal/hcp/registry/json.go @@ -2,6 +2,7 @@ package registry import ( "context" + "fmt" "path/filepath" "github.com/hashicorp/hcl/v2" @@ -27,8 +28,17 @@ func NewJSONMetadataRegistry(config *packer.Core) (*JSONMetadataRegistry, hcl.Di } for _, b := range config.Template.Builders { + buildName := b.Name + + // By default, if the name is unspecified, it will be assigned the type + // + // If the two are different, we can compose the HCP build name from both + if b.Name != b.Type { + buildName = fmt.Sprintf("%s.%s", b.Type, b.Name) + } + // Get all builds slated within config ignoring any only or exclude flags. - bucket.RegisterBuildForComponent(packer.HCPName(b)) + bucket.RegisterBuildForComponent(buildName) } return &JSONMetadataRegistry{ @@ -57,16 +67,16 @@ func (h *JSONMetadataRegistry) PopulateIteration(ctx context.Context) error { } // StartBuild is invoked when one build for the configuration is starting to be processed -func (h *JSONMetadataRegistry) StartBuild(ctx context.Context, buildName string) error { - return h.bucket.startBuild(ctx, buildName) +func (h *JSONMetadataRegistry) StartBuild(ctx context.Context, build sdkpacker.Build) error { + return h.bucket.startBuild(ctx, build.Name()) } // CompleteBuild is invoked when one build for the configuration has finished func (h *JSONMetadataRegistry) CompleteBuild( ctx context.Context, - buildName string, + build sdkpacker.Build, artifacts []sdkpacker.Artifact, buildErr error, ) ([]sdkpacker.Artifact, error) { - return h.bucket.completeBuild(ctx, buildName, artifacts, buildErr) + return h.bucket.completeBuild(ctx, build.Name(), artifacts, buildErr) } diff --git a/internal/hcp/registry/null_registry.go b/internal/hcp/registry/null_registry.go index fba9db4be..dbab4197d 100644 --- a/internal/hcp/registry/null_registry.go +++ b/internal/hcp/registry/null_registry.go @@ -13,13 +13,13 @@ func (r nullRegistry) PopulateIteration(context.Context) error { return nil } -func (r nullRegistry) StartBuild(context.Context, string) error { +func (r nullRegistry) StartBuild(context.Context, sdkpacker.Build) error { return nil } func (r nullRegistry) CompleteBuild( ctx context.Context, - buildName string, + build sdkpacker.Build, artifacts []sdkpacker.Artifact, buildErr error, ) ([]sdkpacker.Artifact, error) { diff --git a/internal/hcp/registry/registry.go b/internal/hcp/registry/registry.go index e9aa3f9f1..16e82eb7f 100644 --- a/internal/hcp/registry/registry.go +++ b/internal/hcp/registry/registry.go @@ -14,8 +14,8 @@ import ( type Registry interface { //Configure(packer.Handler) PopulateIteration(context.Context) error - StartBuild(context.Context, string) error - CompleteBuild(ctx context.Context, buildName string, artifacts []sdkpacker.Artifact, buildErr error) ([]sdkpacker.Artifact, error) + StartBuild(context.Context, sdkpacker.Build) error + CompleteBuild(ctx context.Context, build sdkpacker.Build, artifacts []sdkpacker.Artifact, buildErr error) ([]sdkpacker.Artifact, error) } // New instanciates the appropriate registry for the Packer configuration template type. diff --git a/packer/core.go b/packer/core.go index 40b6560ed..e8da2b2ee 100644 --- a/packer/core.go +++ b/packer/core.go @@ -268,10 +268,9 @@ func (c *Core) generateCoreBuildProvisioner(rawP *template.Provisioner, rawName // This is used for json templates to launch the build plugins. // They will be prepared via b.Prepare() later. -func (c *Core) GetBuilds(opts GetBuildsOptions) ([]packersdk.Build, map[string]string, hcl.Diagnostics) { +func (c *Core) GetBuilds(opts GetBuildsOptions) ([]packersdk.Build, hcl.Diagnostics) { buildNames := c.BuildNames(opts.Only, opts.Except) builds := []packersdk.Build{} - hcpTranslationMap := map[string]string{} diags := hcl.Diagnostics{} for _, n := range buildNames { b, err := c.Build(n) @@ -284,8 +283,6 @@ func (c *Core) GetBuilds(opts GetBuildsOptions) ([]packersdk.Build, map[string]s continue } - hcpTranslationMap[n] = HCPName(c.builds[n]) - // Now that build plugin has been launched, call Prepare() log.Printf("Preparing build: %s", b.Name()) b.SetDebug(opts.Debug) @@ -315,25 +312,7 @@ func (c *Core) GetBuilds(opts GetBuildsOptions) ([]packersdk.Build, map[string]s } } } - return builds, hcpTranslationMap, diags -} - -// HCPName is a helper to get a curated HCP name for a legacy JSON builder. -// -// In order to make the naming scheme between HCL2 and JSON more consistent, -// we implement a similar kind of logic on both template types. -// -// This means that when for HCL2 templates we have a build name formed of -// the source type and the source name, we will do the name here for JSON. -func HCPName(builder *template.Builder) string { - // By default, if the name is unspecified, it will be assigned the type - // - // No need to repeat ourselves here, so we can keep the current behaviour - if builder.Name == builder.Type { - return builder.Name - } - - return fmt.Sprintf("%s.%s", builder.Type, builder.Name) + return builds, diags } // Build returns the Build object for the given name. @@ -441,8 +420,8 @@ func (c *Core) Build(n string) (packersdk.Build, error) { // Return a structure that contains the plugins, their types, variables, and // the raw builder config loaded from the json template - return &CoreBuild{ - Type: n, + cb := &CoreBuild{ + Type: configBuilder.Name, Builder: builder, BuilderConfig: configBuilder.Config, BuilderType: configBuilder.Type, @@ -451,7 +430,13 @@ func (c *Core) Build(n string) (packersdk.Build, error) { CleanupProvisioner: cleanupProvisioner, TemplatePath: c.Template.Path, Variables: c.variables, - }, nil + } + + if configBuilder.Type != configBuilder.Name { + cb.BuildName = configBuilder.Type + } + + return cb, nil } // Context returns an interpolation context. diff --git a/packer/run_interfaces.go b/packer/run_interfaces.go index 0eff3fa4c..418da184c 100644 --- a/packer/run_interfaces.go +++ b/packer/run_interfaces.go @@ -21,7 +21,7 @@ type BuildGetter interface { // GetBuilds return all possible builds for a config. It also starts all // builders. // TODO(azr): rename to builder starter ? - GetBuilds(GetBuildsOptions) ([]packersdk.Build, map[string]string, hcl.Diagnostics) + GetBuilds(GetBuildsOptions) ([]packersdk.Build, hcl.Diagnostics) } type Evaluator interface {