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

Replace deprecated GET api for registry modules #464

Merged
merged 4 commits into from Jul 14, 2022

Conversation

Uk1288
Copy link
Contributor

@Uk1288 Uk1288 commented Jul 14, 2022

Description

The go-client employs the deprecated API (GET /registry-modules/show/:organization_name/:name/:provider) to GET published modules. The API works only for private modules.

This PR replaces the deprecated API with the newer API: GET /organizations/:organization_name/registry-modules/:registry_name/:namespace/:name/:provider which works for both public and private modules.

My initial thought was to replace the RegistryModuleID in Read(ctx context.Context, moduleID RegistryModuleID) (*RegistryModule, error) with:

type RegistryModuleReadOptions struct {
 // Required: The organization the module belongs to
 Organization string
 // Required: The name of the module
 Name string
 // Required: The module provider
 Provider string
 // The namespace of the module. For private modules this is the name of the organization that owns the module
 // Required for public modules
 Namespace string
 // Either public or private. If not provided, defaults to private
 RegistryName RegistryName
}

but this would lead to a breaking change.

To allow for backward compatibility I went with the current solution which just extends the RegistryModuleID.

Testing plan

  1. Create both private and public registry modules
  2. Before the changes, use the Read method to get modules. Private module is returned successfully while public module returns not found
  3. With code changes, Read should return both private and public modules.
  4. It should work with previous RegistryModuleID attributes as well.

External links

Output from tests

Including output from tests may require access to a TFE instance. Ignore this section if you have no environment to test against.

$ TFE_ADDRESS="https://example" TFE_TOKEN="example" TF_ACC="1" go test ./... -v -tags=integration -run TestRegistryModules

=== RUN   TestRegistryModulesRead
=== RUN   TestRegistryModulesRead/with_valid_name_and_provider
2022/07/14 08:36:19 [WARN] Support for using the RegistryModuleID without RegistryName is deprecated as of release 1.5.0 and may be removed in a future version. The preferred method is to include the RegistryName in RegistryModuleID.
2022/07/14 08:36:19 [WARN] Support for using the RegistryModuleID without Namespace is deprecated as of release 1.5.0 and may be removed in a future version. The preferred method is to include the Namespace in RegistryModuleID.
=== RUN   TestRegistryModulesRead/with_valid_name_and_provider/permissions_are_properly_decoded
=== RUN   TestRegistryModulesRead/with_valid_name_and_provider/timestamps_are_properly_decoded
=== RUN   TestRegistryModulesRead/with_complete_registry_module_ID_fields
=== RUN   TestRegistryModulesRead/with_complete_registry_module_ID_fields/permissions_are_properly_decoded
=== RUN   TestRegistryModulesRead/with_complete_registry_module_ID_fields/timestamps_are_properly_decoded
=== RUN   TestRegistryModulesRead/without_a_name
=== RUN   TestRegistryModulesRead/with_an_invalid_name
=== RUN   TestRegistryModulesRead/without_a_provider
=== RUN   TestRegistryModulesRead/with_an_invalid_provider
=== RUN   TestRegistryModulesRead/with_an_invalid_registry_name
=== RUN   TestRegistryModulesRead/without_a_valid_organization
=== RUN   TestRegistryModulesRead/when_the_registry_module_does_not_exist
2022/07/14 08:36:19 [WARN] Support for using the RegistryModuleID without RegistryName is deprecated as of release 1.5.0 and may be removed in a future version. The preferred method is to include the RegistryName in RegistryModuleID.
2022/07/14 08:36:19 [WARN] Support for using the RegistryModuleID without Namespace is deprecated as of release 1.5.0 and may be removed in a future version. The preferred method is to include the Namespace in RegistryModuleID.
--- PASS: TestRegistryModulesRead (1.56s)
    --- PASS: TestRegistryModulesRead/with_valid_name_and_provider (0.12s)
        --- PASS: TestRegistryModulesRead/with_valid_name_and_provider/permissions_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesRead/with_valid_name_and_provider/timestamps_are_properly_decoded (0.00s)
    --- PASS: TestRegistryModulesRead/with_complete_registry_module_ID_fields (0.13s)
        --- PASS: TestRegistryModulesRead/with_complete_registry_module_ID_fields/permissions_are_properly_decoded (0.00s)
        --- PASS: TestRegistryModulesRead/with_complete_registry_module_ID_fields/timestamps_are_properly_decoded (0.00s)
    --- PASS: TestRegistryModulesRead/without_a_name (0.00s)
    --- PASS: TestRegistryModulesRead/with_an_invalid_name (0.00s)
    --- PASS: TestRegistryModulesRead/without_a_provider (0.00s)
    --- PASS: TestRegistryModulesRead/with_an_invalid_provider (0.00s)
    --- PASS: TestRegistryModulesRead/with_an_invalid_registry_name (0.00s)
    --- PASS: TestRegistryModulesRead/without_a_valid_organization (0.00s)
    --- PASS: TestRegistryModulesRead/when_the_registry_module_does_not_exist (0.11s)
=== RUN   TestRegistryModulesDelete

Dependent PR

#460 is blocked by this PR

Copy link
Contributor

@sebasslash sebasslash left a comment

Looks good to me 👍

@Uk1288 Uk1288 merged commit 0f7b645 into main Jul 14, 2022
6 checks passed
@Uk1288 Uk1288 deleted the uk1288-replace-deprecated-api branch Jul 14, 2022
@github-actions
Copy link

github-actions bot commented Jul 14, 2022

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.

None yet

2 participants