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

Data source for packer registry #169

Merged
merged 40 commits into from
Aug 6, 2021
Merged

Data source for packer registry #169

merged 40 commits into from
Aug 6, 2021

Conversation

azr
Copy link
Contributor

@azr azr commented Jul 21, 2021

🛠️ Description

This add a new data source block that will display data from the Packer Registry.

⚠️ I also had to fix a few references for consul things after updating the dependency.

🚢 Release Note

Release note for CHANGELOG:

* provider: add data source for packer registry

🏗️ Acceptance tests

  • Are there any feature flags that are required to use this functionality? > Nope !
  • 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_dataSourcePacker'      
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -run=TestAcc_dataSourcePacker -timeout 120m
?       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     2.709s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/input      3.274s [no tests to run]
=== RUN   TestAcc_dataSourcePacker
--- PASS: TestAcc_dataSourcePacker (9.86s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider   10.540s

...

@azr azr marked this pull request as ready for review July 21, 2021 16:09
@azr azr requested review from a team as code owners July 21, 2021 16:09
@azr azr requested a review from a team July 21, 2021 16:09
Copy link
Contributor

@SwampDragons SwampDragons 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 to me, though I haven't had a chance to test it. How are resources for this provider normally documented? I don't see any doc elements added here, but it's possible they're generated somewhere else and I'm just missing context.

internal/provider/data_source_packer_image.go Outdated Show resolved Hide resolved
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.

Looking good! Had a few questions and comments. Also, you can generate docs by running go generate ./.... Not sure why the PR checks didn't catch that 💭

internal/clients/hvn_route.go Outdated Show resolved Hide resolved
internal/provider/data_source_packer_image.go Outdated Show resolved Hide resolved
internal/provider/data_source_packer_image.go Outdated Show resolved Hide resolved
@azr azr marked this pull request as draft July 27, 2021 11:48
@azr

This comment has been minimized.

Copy link
Contributor

@nywilken nywilken 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 really good.

@azr azr force-pushed the data_source_packer_registry branch from 2dfed66 to 8063413 Compare July 27, 2021 15:06
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! This is definitely close. Just had an open question about the ID field.

I also wasn't able to run the acceptance test ([409] GetChannel default &{Code:10 Details:[] Error:error querying bucket from db Message:error querying bucket from db}), I'm guessing because it still needs a pre-config step that creates the image.

go.mod Outdated Show resolved Hide resolved
internal/provider/data_source_packer_image_iteration.go Outdated Show resolved Hide resolved
internal/provider/data_source_packer_image_iteration.go Outdated Show resolved Hide resolved
return nil
}

func setLocationData(d *schema.ResourceData, loc *sharedmodels.HashicorpCloudLocationLocation) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

Ooo, could make this a shared helper 👀

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 maybe do that in another PR, if time !

@azr
Copy link
Contributor Author

azr commented Aug 3, 2021

Thanks for reviewing ! 🙂 I still need to make the docs page look good and to fix the acc tests.

@azr
Copy link
Contributor Author

azr commented Aug 3, 2021

Hello there, Acc tests are almost working, I just need to fix an endpoint on our end !

go.mod Outdated Show resolved Hide resolved
@azr
Copy link
Contributor Author

azr commented Aug 4, 2021

🥳 Super cool, acc tests pass ! Are we good with docs ? 🙂

@azr azr force-pushed the data_source_packer_registry branch from 7c25fed to b835c06 Compare August 4, 2021 12:45
@azr azr force-pushed the data_source_packer_registry branch from cc8011c to 6343175 Compare August 5, 2021 10:49
@azr
Copy link
Contributor Author

azr commented Aug 5, 2021

We're good with docs ! :shipit:

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.

This is so close! Last step is to upsert one more dependency (channel) and then add a handler to clean up all the upserted dependencies.

},
Schema: map[string]*schema.Schema{
// Required inputs
"bucket": {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any plans to make bucket and channel resources so this can all be managed by TF?

upsertBucket(t, bucket)
upsertIteration(t, bucket, fingerprint)
itID := getIterationIDFromFingerPrint(t, bucket, fingerprint)
upsertBuild(t, bucket, fingerprint, itID)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've been getting an error when running this test:

{"error":"could not load the channel. something in the request is invalid. ","code":3,"message":"could not load the channel. something in the request is invalid. ","details":[]}
2021/08/05 12:05:49 [WARN] Got error running Terraform: exit status 1

I believe it's because the channel hasn't been upserted here.

Copy link
Contributor

Choose a reason for hiding this comment

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

@azr this issue has been address in #180 with the addition of the createChannel and updateChannel calls.

resource.Test(t, resource.TestCase{
PreCheck: func() { testAccPreCheck(t, false) },
ProviderFactories: providerFactories,

Copy link
Contributor

Choose a reason for hiding this comment

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

Since this test upserts some test data that TF doesn't manage directly, we'll need an extra step to clean them up. You can add CheckDestroy: testAccPackerCheckDestroy here, then implement the CheckDestroy function to remove all the upserted data and verify the data source is deleted (peering example)

Copy link
Contributor

Choose a reason for hiding this comment

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

@azr this has been implemented in #180 which @bcmdarroch has reviewed and approved. The change contain a few additional error checks, and a potential bug fix when requesting a channel that has not been assigned a complete iteration.

I confirmed the panic using the provider on an exiting channel with no assigned iteration, and confirmed that the fix errors as expected. Merging the changes as they are for now so that we can get this in. But please follow up on the changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks Wilken !

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.

Just tested with the fixes in #180 and saw the acceptance test pass! 🎉 Let's get this in!

* Add CheckDestroy step for Packer Data Source Test

This change adds a few new helper functions for deleting resources on
the HCP Packer registry under tests.

Test Results After Change
```
make testacc TESTARGS='-count=1 -run=TestAcc_dataSourcePacker'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -count=1
-run=TestAcc_dataSourcePacker -timeout 120m
?       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
0.172s [no tests to run]
testing: warning: no tests to run
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/input
0.127s [no tests to run]
=== RUN   TestAcc_dataSourcePacker
--- PASS: TestAcc_dataSourcePacker (6.47s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider
6.815s
```

* Fix panic when requesting a channel that has no assigned iteration

After change
```
    data_source_packer_image_iteration_test.go:284: Step 1/1 error: Error running pre-apply refresh: exit status 1

        Error: no iteration information found for the specified channel production

          with data.hcp_packer_image_iteration.alpine,
          on terraform_plugin_test.tf line 4, in data "hcp_packer_image_iteration" "alpine":
           4:   data "hcp_packer_image_iteration" "alpine" {

--- FAIL: TestAcc_dataSourcePacker (2.68s)
FAIL
FAIL    github.com/hashicorp/terraform-provider-hcp/internal/provider   3.246s
FAIL
make: *** [testacc] Error 1
```

* Update error handling for resource creation

This change adds a new updateChannel function to handle the updating of
a test channel that may have been created in a previous run, but not
cleaned up.
@nywilken nywilken merged commit 4146ea6 into main Aug 6, 2021
@nywilken nywilken deleted the data_source_packer_registry branch August 6, 2021 18:46
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