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 no-code endpoint for fetching registry module variables. #979

Merged
merged 5 commits into from
Sep 30, 2024

Conversation

paladin-devops
Copy link
Contributor

@paladin-devops paladin-devops commented Sep 23, 2024

Description

This PR adds support to go-tfe for the HCP Terraform endpoint to retrieve a no-code module's variables.

Testing plan

  1. Create a no-code module with at least one required input variable.
  2. Get the ID of the no-code module.
    • expected format: nocode-abcdefgh12345678
  3. Set the environment variable NO_CODE_MODULE_ID to be that ID.
  4. Run go test -run TestRegistryNoCodeModule -v ./..., with the other necessary testing environment variables.

Test Output

=== RUN   TestRegistryNoCodeModulesReadVariables
--- PASS: TestRegistryNoCodeModulesReadVariables (7.94s)
=== RUN   TestRegistryNoCodeModulesReadVariables/happy_path
    --- PASS: TestRegistryNoCodeModulesReadVariables/happy_path (7.66s)
PASS

@paladin-devops paladin-devops self-assigned this Sep 23, 2024
@paladin-devops paladin-devops force-pushed the no-code-read-registry-module-variables branch from 4a7bf25 to 966f2a1 Compare September 23, 2024 21:58
// NOTE: At the time of authoring this comment, the API endpoint to fetch
// registry module variables does not support pagination. This field is
// included to satisfy jsonapi unmarshaler implementation here:
// https://github.com/hashicorp/go-tfe/blob/3d29602707fa4b10469d1a02685644bd159d3ccc/tfe.go#L859
Copy link
Contributor Author

@paladin-devops paladin-devops Sep 23, 2024

Choose a reason for hiding this comment

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

I think this could be a bug in the unmarshaler implementation. In the case of the change in this PR, Items is a slice, and in the unmarshalResponse function, it should be handled by jsonapi.UnmarshalManyPayload. However, without the *Pagination field of this struct present (which should not actually be here, because the backend service doesn't actually return a *Pagination) in the response), the conditional here sends the request into the jsonapi handler UnmarshalPayload, which is documented to support "single request payloads only", leading an attempt to decode a slice to fail.

For now, just including the *Pagination field here is a workaround. But if this issue is fixed, then it can be removed (or if pagination is implemented on the backend, it should be kept).

Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great callout, I'll create a ticket to look into this. 👍

@paladin-devops paladin-devops force-pushed the no-code-read-registry-module-variables branch 2 times, most recently from 9522682 to a0e1538 Compare September 25, 2024 19:36
@paladin-devops paladin-devops marked this pull request as ready for review September 25, 2024 19:36
@paladin-devops paladin-devops force-pushed the no-code-read-registry-module-variables branch from a0e1538 to 3eae2c2 Compare September 26, 2024 14:59
@paladin-devops paladin-devops requested a review from a team September 26, 2024 16:11
Copy link
Contributor

@sebasslash sebasslash left a comment

Choose a reason for hiding this comment

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

One minor change to the changelog, a few extra comments and you're good to go :)

CHANGELOG.md Outdated Show resolved Hide resolved
// NOTE: At the time of authoring this comment, the API endpoint to fetch
// registry module variables does not support pagination. This field is
// included to satisfy jsonapi unmarshaler implementation here:
// https://github.com/hashicorp/go-tfe/blob/3d29602707fa4b10469d1a02685644bd159d3ccc/tfe.go#L859
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a great callout, I'll create a ticket to look into this. 👍

registry_no_code_module_integration_test.go Outdated Show resolved Hide resolved
registry_no_code_module.go Show resolved Hide resolved
@@ -160,6 +202,14 @@ type RegistryNoCodeModuleReadOptions struct {
Include []RegistryNoCodeModuleIncludeOpt `url:"include,omitempty"`
}

type RegistryNoCodeModuleReadVariablesOptions struct {
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's add a GODOC comment here as well

@paladin-devops paladin-devops merged commit f3328da into main Sep 30, 2024
7 checks passed
@paladin-devops paladin-devops deleted the no-code-read-registry-module-variables branch September 30, 2024 17:33
Copy link

Reminder to the contributor that merged this PR: if your changes have added important functionality or fixed a relevant bug, open a follow-up PR to update CHANGELOG.md with a note on your changes.

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.

2 participants