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

[HPR-859] Update resource schema to use a single iteration block #446

Conversation

nywilken
Copy link
Member

@nywilken nywilken commented Jan 31, 2023

🛠️ Description

The current implementation relies on the use of an extra iteration_assignment block to denote what iteration to assign to a channel. Instead of defining a separate block this change uses the single iteration block to denote the state of a channel. Modifying the iteration block will trigger the respective updates to the state of a channel. Removing the iteration block will remove the assigned iteration from the channel.

Potentially Supersedes #435

🏗️ 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=TestAccPackerChannel'

...

@nywilken nywilken requested a review from a team as a code owner January 31, 2023 21:39
@nywilken nywilken requested a review from a team January 31, 2023 21:39
@nywilken nywilken force-pushed the nywilken/packer-channel-refactor branch from 430a0e3 to 7ec2a73 Compare January 31, 2023 21:42
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.

Saw some odd test behavior 🤔

These tests passed.

--- PASS: TestAcc_dataSourcePacker_revokedIteration (163.70s)
--- PASS: TestAcc_dataSourcePackerImage (7.83s)
--- PASS: TestAcc_dataSourcePackerImage_revokedIteration (13.53s)
--- PASS: TestAcc_dataSourcePackerImage_channelAndIterationIDReject (4.87s)
--- PASS: TestAcc_dataSourcePackerImage_channelAccept (11.32s)
--- PASS: TestAcc_dataSourcePackerIteration (6.66s)
--- PASS: TestAcc_dataSourcePackerIteration_revokedIteration (16.26s)
--- PASS: TestAccPackerChannel (10.70s)
--- PASS: TestAccPackerChannel_AssignedIteration (12.90s)
--- PASS: TestAccPackerChannel_UpdateAssignedIteration (23.41s)
--- PASS: TestAccPackerChannel_UpdateAssignedIterationWithFingerprint (6.29s)

But a few of the passing tests included errors in the logs.
TestAcc_dataSourcePackerImage_channelAndIterationIDReject:

[WARN]  sdk.helper_resource: Error running Terraform CLI command: test_name=TestAcc_dataSourcePackerImage_channelAndIterationIDReject test_step_number=1 test_terraform_path=/Users/... test_working_directory=/var/folders/...
   error=
  | exit status 1
  | 
  | Error: Invalid combination of arguments
  | 
  |   with data.hcp_packer_image.arch-btw,
  |   on terraform_plugin_test.tf line 6, in data "hcp_packer_image" "arch-btw":
  |    6: 		iteration_id = "234567"
  | 
  | "iteration_id": only one of `channel,iteration_id` can be specified, but
  | `channel,iteration_id` were specified.

TestAcc_dataSourcePackerImage:

    error=
  | exit status 1
  | 
  | Error: Unable to load image (region: us-east-1, cloud: aws, iteration: 01GR70830X8A1ZKYJYDGFPPNYH, component_type: amazon-ebs.do-not-exist).
  | 
  |   with data.hcp_packer_image.foo,
  |   on terraform_plugin_test.tf line 7, in data "hcp_packer_image" "foo":
  |    7: 	data "hcp_packer_image" "foo" {
  | 
   test_name=TestAcc_dataSourcePackerImage

And TestAcc_dataSourcePacker was the only test that actually failed. I think this might be running into a bug in provider_test, so I can take a look at that and report back.

    provider_test.go:94: [{1 You may experience issues using HCP. HCP is reporting the following:
        
        HCP API:         operational
        HCP Consul:      degraded_performance
        HCP Packer:      operational
        HCP Vault:       operational
        HCP Portal:      operational
        
        Please check https://status.hashicorp.com for more details. []}]
--- FAIL: TestAcc_dataSourcePacker (1.36s)

Type: schema.TypeString,
Optional: true,
Computed: true,
ExactlyOneOf: []string{"iteration.0.id", "iteration.0.fingerprint", "iteration.0.incremental_version"},
Copy link
Contributor

Choose a reason for hiding this comment

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

To clarify what's being enforced here, there can only be one of id or fingerprint or incremental_version set at a time?

Copy link
Member Author

@nywilken nywilken Feb 1, 2023

Choose a reason for hiding this comment

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

Yea, that’s correct. A practitioner should only be allowed to use one of the attributes for iteration assignment.

Allowing the ability to set multiple seems confusing and exposes a limitation in the upstream API where if multiple params are in the request the id take precedence. Which is the reason why I use HasChanges to only select the input that has changed before posting to the API.

@nywilken
Copy link
Member Author

nywilken commented Feb 1, 2023

Saw some odd test behavior 🤔

These tests passed.

--- PASS: TestAcc_dataSourcePacker_revokedIteration (163.70s)
--- PASS: TestAcc_dataSourcePackerImage (7.83s)
--- PASS: TestAcc_dataSourcePackerImage_revokedIteration (13.53s)
--- PASS: TestAcc_dataSourcePackerImage_channelAndIterationIDReject (4.87s)
--- PASS: TestAcc_dataSourcePackerImage_channelAccept (11.32s)
--- PASS: TestAcc_dataSourcePackerIteration (6.66s)
--- PASS: TestAcc_dataSourcePackerIteration_revokedIteration (16.26s)
--- PASS: TestAccPackerChannel (10.70s)
--- PASS: TestAccPackerChannel_AssignedIteration (12.90s)
--- PASS: TestAccPackerChannel_UpdateAssignedIteration (23.41s)
--- PASS: TestAccPackerChannel_UpdateAssignedIterationWithFingerprint (6.29s)

But a few of the passing tests included errors in the logs. TestAcc_dataSourcePackerImage_channelAndIterationIDReject:

[WARN]  sdk.helper_resource: Error running Terraform CLI command: test_name=TestAcc_dataSourcePackerImage_channelAndIterationIDReject test_step_number=1 test_terraform_path=/Users/... test_working_directory=/var/folders/...
   error=
  | exit status 1
  | 
  | Error: Invalid combination of arguments
  | 
  |   with data.hcp_packer_image.arch-btw,
  |   on terraform_plugin_test.tf line 6, in data "hcp_packer_image" "arch-btw":
  |    6: 		iteration_id = "234567"
  | 
  | "iteration_id": only one of `channel,iteration_id` can be specified, but
  | `channel,iteration_id` were specified.

TestAcc_dataSourcePackerImage:

    error=
  | exit status 1
  | 
  | Error: Unable to load image (region: us-east-1, cloud: aws, iteration: 01GR70830X8A1ZKYJYDGFPPNYH, component_type: amazon-ebs.do-not-exist).
  | 
  |   with data.hcp_packer_image.foo,
  |   on terraform_plugin_test.tf line 7, in data "hcp_packer_image" "foo":
  |    7: 	data "hcp_packer_image" "foo" {
  | 
   test_name=TestAcc_dataSourcePackerImage

And TestAcc_dataSourcePacker was the only test that actually failed. I think this might be running into a bug in provider_test, so I can take a look at that and report back.

    provider_test.go:94: [{1 You may experience issues using HCP. HCP is reporting the following:
        
        HCP API:         operational
        HCP Consul:      degraded_performance
        HCP Packer:      operational
        HCP Vault:       operational
        HCP Portal:      operational
        
        Please check https://status.hashicorp.com for more details. []}]
--- FAIL: TestAcc_dataSourcePacker (1.36s)

Is this error generated by the TestAcc_dataSourcePackerImage_channelAndIterationIDReject tests? It might be explicitly checking that the datasource errors when using conflicting input attributes.

@nywilken nywilken changed the title Update resource schema to use a single iteration block [HPR-859] Update resource schema to use a single iteration block Feb 1, 2023
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 like my test failures were related to an underlying provider bug, which I've just fixed: #448

This looks good to me! Glad you dug into this and found the best TF guardrails for this attribute. 🌞

@nywilken
Copy link
Member Author

nywilken commented Feb 9, 2023

This looks good to me! Glad you dug into this and found the best TF guardrails for this attribute.

Thank you @bcmdarroch I appreciate you sticking with me on the two implementations and for your insight. I'm going to merge this change into the other branch, and then rebase/squash #435 to get this change in.

@nywilken nywilken force-pushed the nywilken/add-packer-channel-resource branch from f066cc1 to 76b872b Compare February 9, 2023 16:50
@nywilken nywilken requested a review from a team as a code owner February 9, 2023 16:50
The current implementation relies on the use of an extra
iteration_assignment block to denote what iteration to assign to a
channel. Instead of defining a separate block this change uses the
single iteration block to denote the state of a channel. Modifying the
iteration block will trigger the respective updates to the state of a
channel. Removing the iteration block will remove the assigned iteration
from the channel.
@nywilken nywilken force-pushed the nywilken/packer-channel-refactor branch from 7ec2a73 to b6b9aa7 Compare February 9, 2023 16:56
@nywilken nywilken merged commit f559ad4 into nywilken/add-packer-channel-resource Feb 9, 2023
@nywilken nywilken deleted the nywilken/packer-channel-refactor branch February 9, 2023 18:57
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.

None yet

2 participants