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

Add component_type optional argument #347

Merged

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Jul 7, 2022

to support specifying the exact build image when multiple images exist in the same provider and region for a given iteration

๐Ÿ› ๏ธ Description

Currently, there's a limitation in the hcp_packer_image data source:
https://registry.terraform.io/providers/hashicorp/hcp/latest/docs/data-sources/packer_image

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.

๐Ÿšข Release Note

Release note for CHANGELOG:

* datasource/hcp_packer_image: Add `component_type` optional argument [GH-347]

๐Ÿ—๏ธ 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"
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -run=TestAcc_dataSourcePackerImage -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
--- PASS: TestAcc_dataSourcePackerImage (17.49s)
=== RUN   TestAcc_dataSourcePackerImage_revokedIteration
--- PASS: TestAcc_dataSourcePackerImage_revokedIteration (16.24s)
=== RUN   TestAcc_dataSourcePackerImage_channelAndIterationIDReject
--- PASS: TestAcc_dataSourcePackerImage_channelAndIterationIDReject (7.25s)
=== RUN   TestAcc_dataSourcePackerImage_channelAccept
--- PASS: TestAcc_dataSourcePackerImage_channelAccept (12.05s)
PASS
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	53.669s

to support specifying the exact build image when multiple images exist in the same provider and region for a given iteration
@smaeda-ks smaeda-ks requested a review from a team as a code owner July 7, 2022 15:14
@smaeda-ks smaeda-ks requested a review from a team July 7, 2022 15:14
internal/provider/data_source_packer_image.go Outdated Show resolved Hide resolved
docs/data-sources/packer_image.md Outdated Show resolved Hide resolved
docs/data-sources/packer_image.md Outdated Show resolved Hide resolved
@smaeda-ks smaeda-ks changed the title [WIP] Add component_type optional argument Add component_type optional argument Jul 7, 2022
Copy link
Contributor

@bcmdarroch bcmdarroch 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 to me, but I'm wondering how this fits with the latest change, adding the optional channel. Is this a truly optional filter? Or is it validated like channel and iteration id, where one or the other must be provided?

@smaeda-ks
Copy link
Contributor Author

smaeda-ks commented Jul 8, 2022

Hi @bcmdarroch, thanks for asking! My idea is to bring this new component_type argument as a truly optional filter. If this argument is not specified, the value fallback to the default string value which is "" and we passthrough in this case (works exactly the same as it is today):
https://github.com/hashicorp/terraform-provider-hcp/pull/347/files#diff-75b83f67aad514969dfcdf9ffe45fd2a651b5dbea3d7996c5e94794a555b8726R208-R211

and because channel and iteration_id are checked separately before this for block, they don't interact with this new component_type optional argument.

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.

This looks good and it's a simple way to solve the problem with two images in the same region + iteration. Thanks for taking care of that!

I think the Packer team still needs to take the time to think about other possible ways to solve this. One where the consumer doesn't need to know the source build identifier in the Packer template.

templates/data-sources/packer_image.md.tmpl Outdated Show resolved Hide resolved
@sylviamoss
Copy link
Contributor

We will port over this change to the equivalent Packer data source. Let me know in case you are interested in doing it.

@smaeda-ks smaeda-ks force-pushed the smaeda-ks/packer-image-optional-arg branch from a35adb5 to 6b895f9 Compare July 11, 2022 10:07
@smaeda-ks
Copy link
Contributor Author

@sylviamoss Thank you! As for the Packer data source, it seems straightforward to apply this change there so I might be interested in that work for sure! I'll make a PR there later.

@bcmdarroch
Copy link
Contributor

Gotcha, makes sense. Once Packer team approves I'll go ahead and get this merged.

@sylviamoss
Copy link
Contributor

@bcmdarroch already approved from our side! ๐ŸŽ‰ I will merge it

@sylviamoss sylviamoss merged commit c249cf1 into hashicorp:main Jul 12, 2022
aidan-mundy pushed a commit that referenced this pull request Sep 8, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants