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

Keep failed resources, add state output value #331

Merged
merged 13 commits into from
Jun 28, 2022

Conversation

jjti
Copy link
Contributor

@jjti jjti commented Jun 18, 2022

This is the same couple changes as the other PR targeting Consul but applied to networks and Vault as well:

  1. keep failed resources in state
    • this helps for cleaning of resources
  2. export resource state as an output value

closes: #222

🛠️ Description

🚢 Release Note

Release note for CHANGELOG:

* resource/vault: keep failed clusters, export state [GH-331]
* resource/hvn: keep failed networks/peerings, export state [GH-331]

🏗️ 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=TestAccAwsHvnOnly'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test ./internal/... -v -run=TestAccAwsHvnOnly -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   TestAccAwsHvnOnly
--- PASS: TestAccAwsHvnOnly (163.97s)
PASS
ok      github.com/hashicorp/terraform-provider-hcp/internal/provider   164.359s


→ make testacc TESTARGS='-run=TestAccHvnRoute'  
==> Checking that code complies with gofmt requirements...
...
=== RUN   TestAccHvnRoute
    resource_hvn_route_test.go:82: TestCase error running init: exit status 1
        
        Error: Incompatible provider version
        
        Provider registry.terraform.io/hashicorp/aws v2.64.0 does not have a package
        available for your current platform, darwin_arm64.

hitting issues with a provider version on the route test

@jjti jjti changed the title Stop purging failed resources from state, export state output value Keep failed resources, export state output value Jun 18, 2022
@jjti jjti changed the title Keep failed resources, export state output value Keep failed resources, add state output value Jun 18, 2022
@jjti jjti requested a review from bcmdarroch June 20, 2022 19:15
@jjti jjti marked this pull request as ready for review June 20, 2022 19:15
@jjti jjti requested review from a team June 20, 2022 19:15
@jjti jjti requested a review from a team as a code owner June 20, 2022 19:15
@jjti jjti requested review from tcrayford and markgacoka and removed request for a team June 20, 2022 19:15
// state
if hvn.State == networkmodels.HashicorpCloudNetwork20200907NetworkStateFAILED {
// The HVN has already been deleted, remove from state.
if hvn.State == networkmodels.HashicorpCloudNetwork20200907NetworkStateDELETED {
Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking we should add this check to all the Reads, but it appears that not all the resources have a DELETED state. So I guess we'll just make sure to check on any resource with that state. 👍 (which you've done!)

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.

Testing output:

--- PASS: TestAccAwsHvnOnly (170.69s)
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	171.230s

--- PASS: TestAccAwsPeering (733.26s)
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	733.602s

--- PASS: TestAccTGWAttachment (738.99s)
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	739.340s

--- PASS: TestAccVaultCluster (3159.01s)
ok  	github.com/hashicorp/terraform-provider-hcp/internal/provider	3159.483s

I'm having troubles with timeouts on the Azure peering test. But I do think this is safe to merge.

I'll wrap up testing this change against existing resources (the acceptance tests only cover newly created resources) and then hopefully this should be good to go! 🎉

jjtimmons and others added 13 commits June 22, 2022 13:26
- there has been a longstanding issue where resources are leaked in our e2e tests. This is because we purge all failed clusters from state with d.SetId(""). This doesn't make sense for failed clusters/snapshots. Instead, we should store their state... in state
- and undo property sorting - it makes the PR bigger
- otherwise folks need to upload a credit card to help pr fixes
- reduced the size, need to reduce the scale
@JolisaBrownHashiCorp JolisaBrownHashiCorp force-pushed the jjtimmons/keep-failed-resources-export-state branch from 64038e1 to cb7da48 Compare June 22, 2022 17:26
@jjti jjti requested review from a team as code owners June 22, 2022 17:26
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 completed a manual test to verify existing resources are not impacted. I created the following resources using the latest HCP provider version, v0.32.0:

  • AWS HVN
  • AWS network peering
  • AWS transit gateway
  • AWS HVN route
  • Vault cluster
  1. Sample TF state, created on v0.32.0
# Vault cluster in TF state
{
    "mode": "managed",
    "type": "hcp_vault_cluster",
    "name": "test",
    "provider": "provider[\"registry.terraform.io/hashicorp/hcp\"]",
    "instances": [
        {
            "schema_version": 0,
            "attributes": {
                "cloud_provider": "aws",
                "cluster_id": "test-vault-cluster",
                "created_at": "2022-06-22T19:50:41.440Z",
                ...
            },
            "dependencies": [
                "hcp_hvn.test"
            ]
        }
    ]
}
  1. Output after switching to this branch's local binary and running apply
Apply complete! Resources: 0 added, 0 changed, 0 destroyed.

# Vault cluster in TF state, now refreshed with `state`
{
    "mode": "managed",
    "type": "hcp_vault_cluster",
    "name": "test",
    "provider": "provider[\"registry.terraform.io/hashicorp/hcp\"]",
    "instances": [
        {
            "schema_version": 0,
            "attributes": {
                "cloud_provider": "aws",
                "cluster_id": "test-vault-cluster",
                "created_at": "2022-06-22T19:50:41.440Z",
                "state": "RUNNING",
                ...
            },
            "dependencies": [
                "hcp_hvn.test"
            ]
        }
    ]
}

*I tried to test the Azure resources but I'm experiencing timeout issues. I feel confident this is a safe change to merge, and we can address the testing issues separately.

Base automatically changed from jjtimmons/fix-consul-cluster-failed-state to main June 28, 2022 17:42
@jjti jjti merged commit db2ef7b into main Jun 28, 2022
@jjti jjti deleted the jjtimmons/keep-failed-resources-export-state branch June 28, 2022 17:42
aidan-mundy pushed a commit that referenced this pull request Sep 8, 2023
* Fix storing of failed consul cluster/snapshot state

- there has been a longstanding issue where resources are leaked in our e2e tests. This is because we purge all failed clusters from state with d.SetId(""). This doesn't make sense for failed clusters/snapshots. Instead, we should store their state... in state

* Add acc testing

- and undo property sorting - it makes the PR bigger

* Make docs

* Change acc testing tier to development

- otherwise folks need to upload a credit card to help pr fixes

* Fix scale attr

- reduced the size, need to reduce the scale

* Fix size if acc testing of consul cluster

* Update Consul cluster data source to also store state

* Fix brace in cluster def

* go generate docs

* Use state vs cluster/snapshot_state

* Update internal/provider/resource_consul_cluster_test.go

Co-authored-by: Brenna Hewer-Darroch <[email protected]>

* Stop purging failed resources from state, export state output value

* go generate

Co-authored-by: Brenna Hewer-Darroch <[email protected]>
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.

Failed HVN resource is not deleted on terraform destroy
2 participants