hcp_packer_image channel attribute support #339

lbajolet-hashicorp · 2022-06-28T20:46:43Z

🛠️ Description

Add a new channel attribute to the hcp_packer_image data source, so a user can fetch the latest image directly, without having to specify a companion hcp_packer_iteration data source to fetch the build ID from.

🚢 Release Note

* datasource/hcp_packer_image: allow `channel` attribute to get an image [GH-339]

🏗️ Acceptance tests

Are there any feature flags that are required to use this functionality?
Have you added an acceptance test for the functionality being added?
Have you run the acceptance tests on this branch?

Output from acceptance testing:

$ make testacc TESTARGS='-run=TestAcc_dataSourcePackerImage_chan'
==> Checking that code complies with gofmt requirements...
./scripts/gofmtcheck.sh: line 5: gofmt: command not found
TF_ACC=1 go test ./internal/... -v -run=TestAcc_dataSourcePackerImage_chan -timeout 210m
?   	github.com/hashicorp/terraform-provider-hcp/internal/clients	[no test files]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/consul	(cached) [no tests to run]
testing: warning: no tests to run
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/input	(cached) [no tests to run]
=== RUN   TestAcc_dataSourcePackerImage_channelAndIterationIDReject
--- PASS: TestAcc_dataSourcePackerImage_channelAndIterationIDReject (0.08s)
=== RUN   TestAcc_dataSourcePackerImage_channelAccept
--- PASS: TestAcc_dataSourcePackerImage_channelAccept (3.94s)
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	4.026s

JenGoldstrich · 2022-06-28T21:04:49Z

internal/provider/data_source_packer_image_iteration_test.go

@@ -251,6 +251,35 @@ func getIterationIDFromFingerPrint(t *testing.T, bucketSlug string, fingerprint
 	return ok.Payload.Iteration.ID, nil
 }

+func getBuildIDFromIteration(t *testing.T, bucketSlug, iterID, cloudProvider string) (string, error) {


Where is this function called from?

It's called in the channelAccept test: https://github.com/hashicorp/terraform-provider-hcp/pull/339/files#diff-812fcf1f8adbdbf65c25344beec1814ebf9448de4982f3e52479c3700880fdd5R202

sylviamoss

I'm a little concerned about the optional fields in the docs that are not really optional and also the fact that terraform validate can validate those properly.

In regards to hcp_packer_iteration I think we should keep it valid for now while we observe people migrating to use just hcp_packer_image. The iteration data source returns information about the iteration that the image doesn't, I can't tell right now how useful those information are for the user, so I definitely not deprecate right now 🤔

sylviamoss · 2022-06-29T08:29:06Z

docs/data-sources/packer_image.md

+- `channel` (String) Channel that promotes the latest iteration of the image. Either this or `iteration_id` must be specified.
+- `iteration_id` (String) HCP ID of this image. Either this or `channel' must be specified.


I'm concerned about these being optional here. If they are not set this data source won't work and the user will figure out about this requirement with the error message. Is it possible to write this file manually and skip the generation of these two?

This is the plugin used to generate the docs https://github.com/hashicorp/terraform-plugin-docs

Not certain this is possible, the CI enforces the generated docs being the same as the ones committed :/

This is written based on this template https://github.com/hashicorp/terraform-provider-hcp/blob/main/templates/data-sources/packer_image.md.tmpl.
On top, there's a Description which is populated with the schema description. So we need to update that description as well and mention that this data source needs an iteration id or a channel to query the propper image.

internal/provider/data_source_packer_image.go

sylviamoss · 2022-06-29T08:29:52Z

internal/provider/data_source_packer_image.go

+				Description:  "HCP ID of this image. Either this or `channel' must be specified.",
+				Type:         schema.TypeString,
+				Optional:     true,
+				ExactlyOneOf: []string{"channel"},


Note: on terraform validate this one works when none is set but it doesn't when both are set.

➜ terraform validate Success! The configuration is valid. ➜ terraform validate ╷ │ Error: Invalid combination of arguments │ │ with data.hcp_packer_image.source_ami, │ on main.tf line 16, in data "hcp_packer_image" "source_ami": │ 16: data "hcp_packer_image" "source_ami" { │ │ "iteration_id": one of `channel,iteration_id` must be specified ╵ ➜ tf-modules-local git:(tfc_run_task_example) ✗

Here I would expect that the provider would fail if both fields are unset. I think we can have both fields optional and possibly computed (need to think through this a bit more) and fail in the read if neither is set. If both are set then you would fail as well since the fields need to be mutually exclusive.

The problem is slightly more complex here, it fails at plan/apply time (there's a test for the literals-thing, I'm adding one for the static + reference) but not necessarily at validation-time currently, as it only works when none of the fields are computed.
According to @bflad the terraform-plugin-framework will let us write custom validators that we can use for this kind of use-case, but as it stands we cannot do better I'm afraid.

internal/provider/data_source_packer_image.go

sylviamoss · 2022-06-29T09:12:07Z

internal/provider/data_source_packer_image.go

+		}
+	}
+
+	var diags diag.Diagnostics


This variable is never being set

True, that's an artifact from when both were allowed so long as there wasn't a conflict.
I'll remove this

internal/provider/data_source_packer_image_test.go

sylviamoss · 2022-06-29T14:16:26Z

internal/provider/data_source_packer_image.go

+			"iteration_id": {
+				Description:  "HCP ID of this image. Either this or `channel' must be specified.",
+				Type:         schema.TypeString,
+				Optional:     true,


What do you think about making this value Compute and Optionaland setting this as an output of this data source as well? This way, users are able to know what's the iteration being queried

This actually already set but in the output from the build response, in this case, it makes sense to make this one compute.

I agree this one can be promoted to both Optional and Compute since it's either copied from the input, or derived from the channel, I'll do this if the Schema lets me

nywilken · 2022-06-29T14:08:43Z

docs/data-sources/packer_image.md

 - `region` (String) Region this image is stored in, if any.

 ### Optional

+- `channel` (String) Channel that promotes the latest iteration of the image. Either this or `iteration_id` must be specified.


This data source is not actually promoting an image. So this description is misleading

Suggested change

- `channel` (String) Channel that promotes the latest iteration of the image. Either this or `iteration_id` must be specified.

- `channel` (String) The channel associated to this image.

This was already corrected by @sylviamoss in a previous review, the latest reads as "The channel that points to the version of the image being retrieved.", are you OK with their phrasing?

nywilken · 2022-06-29T14:15:51Z

internal/provider/data_source_packer_image.go

 			"region": {
 				Description: "Region this image is stored in, if any.",
 				Type:        schema.TypeString,
 				Required:    true,
 			},
+			// Optional inputs


We can remove this comment as optional inputs have the Optional field set to true.

This comment is just here for the sake of consistency as we already have some at lines 25 (// Required inputs), and 54 (// computed outputs), I think if we remove this one, the rest should go too for the same consistency reasons, what do you think?

nywilken · 2022-06-29T14:51:14Z

internal/provider/data_source_packer_image.go

+				Description:  "HCP ID of this image. Either this or `channel' must be specified.",
+				Type:         schema.TypeString,
+				Optional:     true,
+				ExactlyOneOf: []string{"channel"},


Could we not use ConflictsWith here?

Not in this case as ConflictsWith would not raise an error if both are missing

nywilken · 2022-06-29T14:57:46Z

internal/provider/data_source_packer_image.go

+				Description:  "HCP ID of this image. Either this or `channel' must be specified.",
+				Type:         schema.TypeString,
+				Optional:     true,
+				ExactlyOneOf: []string{"channel"},


Here I would expect that the provider would fail if both fields are unset. I think we can have both fields optional and possibly computed (need to think through this a bit more) and fail in the read if neither is set. If both are set then you would fail as well since the fields need to be mutually exclusive.

internal/provider/data_source_packer_image.go

nywilken · 2022-06-29T15:10:28Z

internal/provider/data_source_packer_image.go

+	channelName := d.Get("channel")
+	iterationID := d.Get("iteration_id")


I believe these need to be type asserted as they are stored as interface{} values.

Suggested change

channelName := d.Get("channel")

iterationID := d.Get("iteration_id")

channelName := d.Get("channel").(string)

iterationID := d.Get("iteration_id").(string)

That said I would recommend creating a new helper function
The following is a mixture of psuedo and Go code.

func getIteration(ctx context.Context, d *schema.ResourceData) (iteration, error) { // validate values for channel and iteration_id // only one can be set. /// if both are unset fail, // if both are set fail. if iterationID is not empty { return clients.GetIterationFromId( ctx, client, loc, bucketName, iterID) } channel, err = clients.GetPackerChannelBySlug( ctx, client, loc, bucketName, chanSlug) if err != nil { return nil, err } channel.Iteration, nil }

nywilken · 2022-06-29T15:11:34Z

internal/provider/data_source_packer_image.go

+	if channel != nil && iteration != nil {
+		return diag.FromErr(fmt.Errorf(
+			"iteration mismatch: channel %s's iteration (%s) is different from the explicitely specified iteration: %s",
+			channel.Slug,
+			channel.Iteration.ID,
+			iteration.ID))
+	}


I'm a little confused by this as channel and iteration appear to be mutually exclusive. Is this check needed?

Well, it used to be before we had the ExactlyOneOf in the Schema, now you're right it's not needed as the configuration attributes cannot be both set to something other than the empty string.

nywilken · 2022-06-29T15:12:09Z

internal/provider/data_source_packer_image.go

+	// Assuming we passed the above check, the rest of the channel is not
+	// used after that,
+	if channel != nil {
+		iteration = channel.Iteration
 	}


By using the helper function you can get rid of this as we will already have the iteration information we are looking for.

sylviamoss · 2022-06-30T09:12:41Z

internal/provider/data_source_packer_image_test.go

+		testAccPackerImageBothChanAndIter,
+		testAccPackerImageBothChanAndIterRef,


internal/provider/data_source_packer_image_test.go

A hcp_packer_image used to rely on the iteration_id, which it could get from a second resource in the plan. However, this information is redundant, as the image's ID can be inferred directly by the image block, if we specify its channel. Therefore, we let a user specify either an iteration_id for compatibility, or a channel so they won't need both information in a single plan.

sylviamoss · 2022-07-01T09:28:42Z

docs/data-sources/packer_image.md

 ```

 ~> **Note:** This data source only returns the first found image's metadata filtered by the given schema values, from the returned list of images associated with the specified iteration. Therefore, if multiple images exist in the same region, it will only pick one of them. If that's the case, you may consider separating your builds into different buckets.

+~> **Note:** The `channel` attribute in this data source will incur a billable request to HCP Packer. This attribute is intended for convenience when using a single image. When sourcing multiple images from a single iteration, the separate data source object for the iteration alternative is preferred for saving costs.


What do you think? I changed to may because there are limited free requests in the standard tier 🤔

Suggested change

~> **Note:** The `channel` attribute in this data source will incur a billable request to HCP Packer. This attribute is intended for convenience when using a single image. When sourcing multiple images from a single iteration, the separate data source object for the iteration alternative is preferred for saving costs.

~> **Note:** The `channel` attribute in this data source may incur a billable request to HCP Packer. This attribute is intended for convenience when using a single image. When sourcing multiple images from a single iteration, the `hcp_packer_iteration` data source

is the alternative for querying a channel just once.

sylviamoss · 2022-07-01T09:33:46Z

docs/data-sources/packer_image.md

+# If sourcing multiple images
+


Suggested change

# If sourcing multiple images

sylviamoss · 2022-07-01T09:39:44Z

docs/data-sources/packer_image.md

+# Alternatively if sourcing a single image
+
+data "hcp_packer_image" "baz" {
+  bucket_name    = "hardened-ubuntu-16-04"
+  cloud_provider = "aws"
+  channel        = "production"
+  region         = "us-east-1"
+}
+
+output "packer-registry-ubuntu-east-1-alt" {
+  value = data.hcp_packer_image.baz.cloud_image_id
+}


Suggested change

# Alternatively if sourcing a single image

data "hcp_packer_image" "baz" {

bucket_name = "hardened-ubuntu-16-04"

cloud_provider = "aws"

channel = "production"

region = "us-east-1"

}

output "packer-registry-ubuntu-east-1-alt" {

value = data.hcp_packer_image.baz.cloud_image_id

}

### Alternatively, if sourcing a single image

data "hcp_packer_image" "baz" {

bucket_name = "hardened-ubuntu-16-04"

cloud_provider = "aws"

channel = "production"

region = "us-east-1"

}

output "packer-registry-ubuntu-east-1-alt" {

value = data.hcp_packer_image.baz.cloud_image_id

}

I'm having a hard time adding the code block because the markdown is getting confused with this comment markdown. I'm suggesting moving the alternative into a separated sub-section and keeping both examples separated because all together looks big and hard to quickly see how it works.

Separating both source examples is a valid alternative, I'll do this

sylviamoss

I suggested some changes to the doc, otherwise LGTM 👍🏼
Great work!

With the introduction of `channel' in hcp_packer_image, we add some more examples in the documentation for the data_source.

A hcp_packer_image used to rely on the iteration_id, which it could get from a second resource in the plan. However, this information is redundant, as the image's ID can be inferred directly by the image block, if we specify its channel. Therefore, we let a user specify either an iteration_id for compatibility, or a channel so they won't need both information in a single plan. This does not make the hcp_iteration entity redundant however, as one can reuse its data for several images, hence why we keep it for now.

lbajolet-hashicorp requested a review from a team as a code owner June 28, 2022 20:46

lbajolet-hashicorp requested a review from a team June 28, 2022 20:46

lbajolet-hashicorp force-pushed the packer_image_channel_support branch from f81915f to c7460a6 Compare June 28, 2022 20:54

JenGoldstrich reviewed Jun 28, 2022

View reviewed changes

sylviamoss reviewed Jun 29, 2022

View reviewed changes

nywilken reviewed Jun 29, 2022

View reviewed changes

lbajolet-hashicorp force-pushed the packer_image_channel_support branch 3 times, most recently from 39e4f15 to 1e6ba48 Compare June 29, 2022 20:28

sylviamoss reviewed Jun 30, 2022

View reviewed changes

internal/provider/data_source_packer_image_test.go Show resolved Hide resolved

lbajolet-hashicorp force-pushed the packer_image_channel_support branch from 1e6ba48 to 1809ea9 Compare June 30, 2022 15:07

sylviamoss reviewed Jul 1, 2022

View reviewed changes

sylviamoss approved these changes Jul 1, 2022

View reviewed changes

docs: add packer_image alts examples

83dac3e

With the introduction of `channel' in hcp_packer_image, we add some more examples in the documentation for the data_source.

lbajolet-hashicorp force-pushed the packer_image_channel_support branch from 1809ea9 to 83dac3e Compare July 4, 2022 13:02

lbajolet-hashicorp merged commit b96faaf into main Jul 5, 2022

lbajolet-hashicorp deleted the packer_image_channel_support branch July 5, 2022 20:20

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hcp_packer_image channel attribute support #339

hcp_packer_image channel attribute support #339

lbajolet-hashicorp commented Jun 28, 2022 •

edited

Loading

JenGoldstrich Jun 28, 2022

lbajolet-hashicorp Jun 28, 2022

sylviamoss left a comment

sylviamoss Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

sylviamoss Jun 29, 2022

sylviamoss Jun 29, 2022

nywilken Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

sylviamoss Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

sylviamoss Jun 29, 2022

sylviamoss Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

nywilken Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

nywilken Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

nywilken Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022

nywilken Jun 29, 2022

nywilken Jun 29, 2022

nywilken Jun 29, 2022

lbajolet-hashicorp Jun 29, 2022 •

edited

Loading

nywilken Jun 29, 2022

sylviamoss Jun 30, 2022

sylviamoss Jul 1, 2022 •

edited

Loading

sylviamoss Jul 1, 2022

sylviamoss Jul 1, 2022

lbajolet-hashicorp Jul 4, 2022

sylviamoss left a comment

		- `channel` (String) Channel that promotes the latest iteration of the image. Either this or `iteration_id` must be specified.
		- `iteration_id` (String) HCP ID of this image. Either this or `channel' must be specified.

	- `channel` (String) Channel that promotes the latest iteration of the image. Either this or `iteration_id` must be specified.
	- `channel` (String) The channel associated to this image.

		channelName := d.Get("channel")
		iterationID := d.Get("iteration_id")

		testAccPackerImageBothChanAndIter,
		testAccPackerImageBothChanAndIterRef,

	~> Note: The `channel` attribute in this data source will incur a billable request to HCP Packer. This attribute is intended for convenience when using a single image. When sourcing multiple images from a single iteration, the separate data source object for the iteration alternative is preferred for saving costs.
	~> Note: The `channel` attribute in this data source may incur a billable request to HCP Packer. This attribute is intended for convenience when using a single image. When sourcing multiple images from a single iteration, the `hcp_packer_iteration` data source
	is the alternative for querying a channel just once.

hcp_packer_image channel attribute support #339

hcp_packer_image channel attribute support #339

Conversation

lbajolet-hashicorp commented Jun 28, 2022 • edited Loading

🛠️ Description

🚢 Release Note

🏗️ Acceptance tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylviamoss left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lbajolet-hashicorp Jun 29, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylviamoss Jul 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sylviamoss left a comment

Choose a reason for hiding this comment

lbajolet-hashicorp commented Jun 28, 2022 •

edited

Loading

lbajolet-hashicorp Jun 29, 2022 •

edited

Loading

sylviamoss Jul 1, 2022 •

edited

Loading