Skip to content
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

hcp-packer-image: add support for channel #11865

Merged
merged 2 commits into from
Jul 13, 2022

Conversation

lbajolet-hashicorp
Copy link
Contributor

In addition to the current way of specifying an image based on an
iteration on HCP Packer, which requires first declaring an iteration,
and then referencing it from the image to build, we add the capacity of
specifying a channel.

This alternative will get the iteration linked to the channel, and is
essentially a more convenient way to get an image's metadata from HCP
Packer.

This commit is essentially a backport from the Terraform HCP provider,
for consistency between the two tools.

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good so far! Docs are not being rendered because the new file is not mentioned in the data source documentation. Anyways, I left a comment about making both fields required so they appear under the required fields in the documentation. Let me know what you think about it.

if d.config.Channel != "" &&
d.config.IterationID != "" {
errs = packersdk.MultiErrorAppend(errs, errors.New(
"`iteration_id` and `channel` cannot both be specified"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"`iteration_id` and `channel` cannot both be specified"))
"`iteration_id` and `channel` cannot both be specified."))

if d.config.Channel == "" &&
d.config.IterationID == "" {
errs = packersdk.MultiErrorAppend(errs, errors.New(
"The `iteration_id` or `channel` must be specified"))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
"The `iteration_id` or `channel` must be specified"))
"The `iteration_id` or `channel` must be specified."))

@@ -0,0 +1,19 @@
<!-- Code generated from the comments of the Config struct in datasource/hcp-packer-image/data.go; DO NOT EDIT MANUALLY -->
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a new file that has to be included in the data source main mdx file to appear in the docs.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll keep that in mind for future changes, though with your next comment, this will remove this file from the commit :)

// Either this or `iteration_id` MUST be set.
//
// Mutually exclusive with `iteration_id`
Channel string `mapstructure:"channel" required:"false"`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Packer let's you set these two as required and at the same time let them not be set. The validation of those required fields is just the one you implemented. I've seen other plugins with mutually exclusive configs that were required.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's good to know, I thought the HCL2 parser would check this, if that's not the case then perfect, it will be clearer if we include them in the required block, with a note stating they're mutually exclusive.

Comment on lines 32 to 44
// The name of the channel to use when retrieving your image.
//
// If using several images from a single iteration, you may prefer
// sourcing an iteration first, and referencing it for subsequent uses,
// as getting the iteration induces a potentially billable request.
//
// Either this or `iteration_id` MUST be set.
//
// Mutually exclusive with `iteration_id`
Channel string `mapstructure:"channel" required:"true"`
// The ID of the iteration to use when retrieving your image
//
// Either this or `channel` MUST be set.
//
// Mutually exclusive with `channel`
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Very true. I'll get that more consistent before we merge this.

Copy link
Contributor

@sylviamoss sylviamoss left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎉 👍🏼

Copy link
Contributor

@JenGoldstrich JenGoldstrich left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, I think you should consider my docs nit change though, let me know what you think!

In addition to the current way of specifying an image based on an
iteration on HCP Packer, which requires first declaring an iteration,
and then referencing it from the image to build, we add the capacity of
specifying a channel.

This alternative will get the iteration linked to the channel, and is
essentially a more convenient way to get an image's metadata from HCP
Packer.

This commit is essentially a backport from the Terraform HCP provider,
for consistency between the two tools.
The comment was mentioning the name of an iteration Id, which is
conflicting.
This commit rephrases the comment to directly specify the ID of the
iteration.
@lbajolet-hashicorp lbajolet-hashicorp force-pushed the hcp_packer_image_channel branch from c259839 to b29be57 Compare July 13, 2022 20:54
@lbajolet-hashicorp lbajolet-hashicorp merged commit 819cc50 into main Jul 13, 2022
@lbajolet-hashicorp lbajolet-hashicorp deleted the hcp_packer_image_channel branch July 13, 2022 21:10
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Aug 13, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants