add support for registry modules without vcs #546

Uk1288 · 2022-07-07T19:45:02Z

Description

This PR adds changes that support creating both private and public registry modules without using a VCS. It also extends the ability to import both private and public registry module by including the namespace and registry-name in the import id.

Remember to:

Update the Change Log
Update the Documentation

Testing plan

With existing code a registry module can only be created via the required VCS block
After code changes, both private and public modules can be added without the vcs block
The import identifier includes the namespace and registry_name and should be able to import private and public modules
Some sample config to test modules:

## should create public module successfully
resource "tfe_registry_module" "public-registry-module-without-vcs" {
  organization         = "an-org"
  namespace          = "terraform-aws-modules"
  module_provider = "aws"
  name                    = "iam"
  registry_name     = "public"
}

## should create private module successfully
resource "tfe_registry_module" "private-registry-module-without-registry-name" {
  organization    = "hashicorp"
  module_provider = "aws"
  name            = "vpc"
}

## should create private module successfully
resource "tfe_registry_module" "private-registry-module-without-vcs" {
  organization    = "hashicorp"
  module_provider = "aws"
  name            = "eks"
  registry_name   = "private"
}

Deprecated import format <ORGANIZATION>/<REGISTRY MODULE NAME>/<REGISTRY MODULE PROVIDER>/<REGISTRY MODULE ID> works for only private module:
#should succeed

terraform import tfe_registry_module.test-private my-org-name/name/provider/mod-qV9JnKRkmtMa4zcA

#should fail

terraform import tfe_registry_module.test-public my-org-name/name/provider/mod-qV9JnKRkmtMa4rzA

New import format <ORGANIZATION>/<REGISTRY_NAME>/<NAMESPACE>/<REGISTRY MODULE NAME>/<REGISTRY MODULE PROVIDER>/<REGISTRY MODULE ID> works for both public and private module:
#should succeed

terraform import tfe_registry_module.test-private my-org-name/private/namespace/name/provider/mod-qV9JnKRkmtMa4zcA

#should succeed

terraform import tfe_registry_module.test-public my-org-name/public/namespace/name/provider/mod-qV9JnKRkmtMa4rzA

External links

Include any links here that might be helpful for people reviewing your PR. If there are none, feel free to delete this section.

Output from acceptance tests

Please run applicable acceptance tests locally and include the output here. See TESTS.md to learn how to run acceptance tests.

If you are an external contributor, your contribution(s) will first be reviewed before running them against the project's CI pipeline.

$ TESTARGS="-run TestAccTFERegistryModule" make testacc

=== RUN   TestAccTFERegistryModule_vcs
--- PASS: TestAccTFERegistryModule_vcs (10.62s)
=== RUN   TestAccTFERegistryModule_emptyVCSRepo
--- PASS: TestAccTFERegistryModule_emptyVCSRepo (0.97s)
=== RUN   TestAccTFERegistryModule_nonVCSPrivateRegistryModuleWithoutRegistryName
--- PASS: TestAccTFERegistryModule_nonVCSPrivateRegistryModuleWithoutRegistryName (7.02s)
=== RUN   TestAccTFERegistryModule_nonVCSPrivateRegistryModuleWithRegistryName
--- PASS: TestAccTFERegistryModule_nonVCSPrivateRegistryModuleWithRegistryName (6.84s)
=== RUN   TestAccTFERegistryModule_publicRegistryModule
--- PASS: TestAccTFERegistryModule_publicRegistryModule (7.12s)
=== RUN   TestAccTFERegistryModuleImport_vcsPrivateRMDeprecatedFormat
--- PASS: TestAccTFERegistryModuleImport_vcsPrivateRMDeprecatedFormat (9.85s)
=== RUN   TestAccTFERegistryModuleImport_vcsPrivateRMRecommendedFormat
--- PASS: TestAccTFERegistryModuleImport_vcsPrivateRMRecommendedFormat (11.60s)
=== RUN   TestAccTFERegistryModuleImport_nonVCSPrivateRM
--- PASS: TestAccTFERegistryModuleImport_nonVCSPrivateRM (11.80s)
=== RUN   TestAccTFERegistryModuleImport_publicRM
--- PASS: TestAccTFERegistryModuleImport_publicRM (7.36s)
=== RUN   TestAccTFERegistryModule_invalidWithBothVCSRepoAndModuleProvider
--- PASS: TestAccTFERegistryModule_invalidWithBothVCSRepoAndModuleProvider (0.71s)
=== RUN   TestAccTFERegistryModule_invalidRegistryName
--- PASS: TestAccTFERegistryModule_invalidRegistryName (2.35s)
=== RUN   TestAccTFERegistryModule_invalidWithModuleProviderAndNoName
--- PASS: TestAccTFERegistryModule_invalidWithModuleProviderAndNoName (0.70s)
=== RUN   TestAccTFERegistryModule_invalidWithModuleProviderAndNoOrganization
--- PASS: TestAccTFERegistryModule_invalidWithModuleProviderAndNoOrganization (0.70s)
=== RUN   TestAccTFERegistryModule_invalidWithNamespaceAndNoRegistryName
--- PASS: TestAccTFERegistryModule_invalidWithNamespaceAndNoRegistryName (0.85s)
=== RUN   TestAccTFERegistryModule_invalidWithRegistryNameAndNoModuleProvider
--- PASS: TestAccTFERegistryModule_invalidWithRegistryNameAndNoModuleProvider (0.83s)
PASS
ok  	github.com/hashicorp/terraform-provider-tfe/tfe	79.993s

Uk1288 · 2022-07-18T16:46:34Z

tfe/resource_tfe_registry_module_test.go

+}
+
+resource "tfe_registry_module" "foobar" {
+	organization    = tfe_organization.foobar.id


these spaces are weird, they show up in my PR but in my VScode, they are all aligned. Not sure what's causing this. I will try running the formatter...any ideas?

Uk1288 · 2022-07-18T16:47:40Z

website/docs/r/registry_module.html.markdown

+terraform import tfe_registry_module.test my-org-name/public/namespace/name/provider/mod-qV9JnKRkmtMa4zcA
+```
+
+**Deprecated** use `<ORGANIZATION NAME>/<REGISTRY MODULE NAME>/<REGISTRY MODULE PROVIDER>/<REGISTRY MODULE ID>` as the import ID. For example:


FYI Pointing out deprecation, to make it obvious when reviewing

Uk1288 · 2022-07-18T16:48:48Z

tfe/resource_tfe_registry_module.go

+				Type:         schema.TypeString,
+				Computed:     true,
+				Optional:     true,
+				ForceNew:     true,


Pointing out ForceNew attribute for reviewing

Uk1288 · 2022-07-18T16:49:01Z

tfe/resource_tfe_registry_module.go

+				Type:         schema.TypeString,
+				Computed:     true,
+				Optional:     true,
+				ForceNew:     true,


Pointing out ForceNew attribute for reviewing

Uk1288 · 2022-07-18T16:49:13Z

tfe/resource_tfe_registry_module.go

+				Type:          schema.TypeString,
+				Computed:      true,
+				Optional:      true,
+				ForceNew:      true,


Pointing out ForceNew attribute for reviewing

brandonc

Some code feedback while I kick the tires...

brandonc · 2022-07-18T21:27:40Z

CHANGELOG.md

@@ -3,7 +3,12 @@
 FEATURES:
 * **New Resource**: `tfe_workspace_variable_set` ([#537](https://github.com/hashicorp/terraform-provider-tfe/pull/537)) adds the ability to assign a variable set to a workspace in a single, flexible resource.

-DEPRECATION NOTICE: The `workspace_ids` argument on `tfe_variable_set` has been labelled as deprecated and should not be used in conjunction with `tfe_workspace_variable_set`.
+ENHANCEMENTS:


You can drop this heading and move the new entry to "FEATURES" above

brandonc · 2022-07-18T21:41:27Z

tfe/resource_tfe_registry_module.go

+	if v, ok := d.GetOk("vcs_repo"); ok {
+		registryModule, err = resourceTFERegistryModuleCreateWithVCS(v, meta)
+	} else {
+		registryModule, err = resourceTFERegistryModuleCreateWithoutVCS(meta, d)
+	}


I like this.

brandonc · 2022-07-18T21:49:08Z

tfe/resource_tfe_registry_module.go

+				Computed:      true,
+				Optional:      true,
+				ForceNew:      true,
+				ExactlyOneOf:  []string{"vcs_repo"},


Can you describe what this validation does? I couldn't find the docs

In this validation I used both ExactlyOneOf and ConflictsWith.

Even though both module_provider and vcs_repo attribute is optional, ExactlyOneOf will raise an error if none of them is specified.

ConflictsWith ensures that either a module_provider or vcs_repo attribute is used but not both. Essentially they are mutually exclusive.

Now that I am rethinking it, seems like using only ExactlyOneOf might suffice. I will test that.

brandonc

Minor feedback after smoke tests. Nice work!!

brandonc · 2022-07-18T22:27:47Z

website/docs/r/registry_module.html.markdown

-  new resource if changed.
+* `vcs_repo` - (Optional) Settings for the registry module's VCS repository. Forces a
+  new resource if changed. One of `vcs_repo` or `module_provider` is required.
+* `module_provider` - (Optional) The provider of the registry module.


I think we should be a little more specific about what input is expected here, since this appears to be a key attribute when vcs is not used. I was slightly confused until I thought more about it and looked at the docs.

Suggested change

* `module_provider` - (Optional) The provider of the registry module.

* `module_provider` - (Optional) Specifies the Terraform provider that this module is used for. For example, "aws"

brandonc · 2022-07-18T22:33:25Z

tfe/resource_tfe_registry_module.go

+		if registryName.(string) == "private" && ok {
+			// Namespace is not allowed for private registry name
+			return nil, fmt.Errorf("Namespace is not allowed for %s registry name", registryName)
+		}
+
+		if registryName.(string) == "public" {
+			if !ok { // Namespace is required for public registry name
+				return nil, fmt.Errorf("Namespace is required for %s registry name", registryName)
+			}
+			options.Namespace = namespace.(string)
+		}
+	}


💅 Are these conditions already handled by RegistryModules.Create ? It might simplify things to just delegate this kind of validation to the client.

Yes they are. I had similar thought as well but I wasn't sure of the validation pattern. I will update this.

brandonc · 2022-07-18T22:36:43Z

tfe/resource_tfe_registry_module.go

@@ -59,39 +67,109 @@ func resourceTFERegistryModule() *schema.Resource {
 					},
 				},
 			},
+			"namespace": {
+				Type:         schema.TypeString,
+				Computed:     true,


I don't think this, or registry_name, or module_provider attributes are Computed any longer since they are input arguments now. Computed is usually reserved for attributes that the user cannot set and are provided by the API (like 'created' timestamps)

brandonc

🎉

Uk1288 force-pushed the uk1288-add-public-modules branch from 1a9b5fe to 27be269 Compare July 7, 2022 20:45

Uk1288 mentioned this pull request Jul 11, 2022

add support for both public and private module creation without VCS hashicorp/go-tfe#460

Merged

Uk1288 marked this pull request as ready for review July 18, 2022 14:33

Uk1288 force-pushed the uk1288-add-public-modules branch from cbe875e to 408d380 Compare July 18, 2022 14:38

Uk1288 commented Jul 18, 2022

View reviewed changes

brandonc reviewed Jul 18, 2022

View reviewed changes

brandonc requested changes Jul 18, 2022

View reviewed changes

Uk1288 requested a review from a team as a code owner July 19, 2022 16:02

Uk1288 added 2 commits July 25, 2022 16:06

add support for registry modules without vcs

b767c09

upgrade go-tfe

786db53

Uk1288 force-pushed the uk1288-add-public-modules branch from dcdb8d8 to 786db53 Compare July 25, 2022 20:07

Uk1288 added 2 commits July 25, 2022 16:10

update CHANGELOG.md

711db17

add ExpectNonEmptyPlan:true to registry modules test

4f61198

brandonc approved these changes Jul 25, 2022

View reviewed changes

Uk1288 merged commit 780d5af into main Jul 25, 2022

Uk1288 deleted the uk1288-add-public-modules branch July 25, 2022 21:32

relusc mentioned this pull request Jul 27, 2022

tfe_registry_modules with VCS connection get replaced on every run #571

Closed

lucymhdavies mentioned this pull request Aug 5, 2022

Support for adding Public Modules to registry #383

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add support for registry modules without vcs #546

add support for registry modules without vcs #546

Uk1288 commented Jul 7, 2022 •

edited

Uk1288 Jul 18, 2022

Uk1288 Jul 18, 2022

Uk1288 Jul 18, 2022

Uk1288 Jul 18, 2022

Uk1288 Jul 18, 2022

brandonc left a comment

brandonc Jul 18, 2022

brandonc Jul 18, 2022

brandonc Jul 18, 2022

Uk1288 Jul 19, 2022

brandonc left a comment

brandonc Jul 18, 2022

brandonc Jul 18, 2022 •

edited

Uk1288 Jul 19, 2022

brandonc Jul 18, 2022

brandonc left a comment

	* `module_provider` - (Optional) The provider of the registry module.
	* `module_provider` - (Optional) Specifies the Terraform provider that this module is used for. For example, "aws"

add support for registry modules without vcs #546

add support for registry modules without vcs #546

Conversation

Uk1288 commented Jul 7, 2022 • edited

Description

Testing plan

External links

Output from acceptance tests

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc Jul 18, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

brandonc left a comment

Choose a reason for hiding this comment

Uk1288 commented Jul 7, 2022 •

edited

brandonc Jul 18, 2022 •

edited