-
Notifications
You must be signed in to change notification settings - Fork 3.3k
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
enabling hcp multibuilds #13328
base: main
Are you sure you want to change the base?
enabling hcp multibuilds #13328
Conversation
7a6f07b
to
8ffabdb
Compare
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.
Left one small nit on a diagnostic message, but besides that, it looks good to me!
8ffabdb
to
367ea52
Compare
367ea52
to
5e2809a
Compare
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.
Some small adjustments on the diagnostics, another small nit on the test code, but besides those, LGTM!
internal/hcp/registry/hcl_test.go
Outdated
expectErr: false, | ||
expectedBuilds: []string{"docker.ubuntu", "build.docker.ubuntu"}, | ||
expectErr: true, | ||
// expectedBuilds: []string{"docker.ubuntu", "build.docker.ubuntu"}, |
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.
You can probably remove this instead of commenting it
internal/hcp/registry/hcl.go
Outdated
Detail: fmt.Sprintf("Two sources are used in the same build block, causing "+ | ||
"a conflict, there must only be one instance of %s", source.String()), | ||
Detail: fmt.Sprintf("Two sources are used in the same build block or "+ | ||
"two build has the same name causing a conflict, there must only "+ |
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.
"two build has the same name causing a conflict, there must only "+ | |
"two builds have the same name, causing a conflict, there must only "+ |
internal/hcp/registry/hcl.go
Outdated
if build.Name == "" { | ||
diags = append(diags, &hcl.Diagnostic{ | ||
Severity: hcl.DiagError, | ||
Summary: "Build name conflicts", |
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 would suggest pointing out at the lack of a build name here instead as the summary
Summary: "Build name conflicts", | |
Summary: "Missing mandatory build name", |
internal/hcp/registry/hcl.go
Outdated
Severity: hcl.DiagError, | ||
Summary: "Build name conflicts", | ||
Subject: &build.HCL2Ref.DefRange, | ||
Detail: "build name is required when using the same source in two different builds", |
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.
Detail: "build name is required when using the same source in two different builds", | |
Detail: "A build name is required when using the same source in two or more different builds", |
hcl2template/types.packer_config.go
Outdated
} | ||
block = build.HCPPackerRegistry | ||
// build block leveled hcp registry is only tolerated when there is only one build block | ||
if len(cfg.Builds) <= 1 { |
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.
Likely not an issue, but if we don't have a build block defined here, what happens? I'm not sure we even reach this code, but I wonder if the check here shouldn't be exactly len(cfg.Builds) == 1
instead of a <=
, otherwise we'll crash on the following statement since cfg.Builds[0]
will be out-of-bounds
721a0b8
to
33e593a
Compare
33e593a
to
0794593
Compare
HCP Packer multibuilds
This Pr will remove the check that prevents user from using multiple build block in HCL2 configuration.
Example
Output:
On the HCP Packer portal:

(Note that it's version 4 since I tested some combination before)