-
Notifications
You must be signed in to change notification settings - Fork 154
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 support for registry modules without vcs #546
Conversation
1a9b5fe
to
27be269
Compare
cbe875e
to
408d380
Compare
} | ||
|
||
resource "tfe_registry_module" "foobar" { | ||
organization = tfe_organization.foobar.id |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FYI Pointing out deprecation, to make it obvious when reviewing
Type: schema.TypeString, | ||
Computed: true, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out ForceNew
attribute for reviewing
Type: schema.TypeString, | ||
Computed: true, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out ForceNew
attribute for reviewing
tfe/resource_tfe_registry_module.go
Outdated
Type: schema.TypeString, | ||
Computed: true, | ||
Optional: true, | ||
ForceNew: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pointing out ForceNew
attribute for reviewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some code feedback while I kick the tires...
CHANGELOG.md
Outdated
@@ -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: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can drop this heading and move the new entry to "FEATURES" above
if v, ok := d.GetOk("vcs_repo"); ok { | ||
registryModule, err = resourceTFERegistryModuleCreateWithVCS(v, meta) | ||
} else { | ||
registryModule, err = resourceTFERegistryModuleCreateWithoutVCS(meta, d) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I like this.
tfe/resource_tfe_registry_module.go
Outdated
Computed: true, | ||
Optional: true, | ||
ForceNew: true, | ||
ExactlyOneOf: []string{"vcs_repo"}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you describe what this validation does? I couldn't find the docs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor feedback after smoke tests. Nice work!!
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
* `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" |
tfe/resource_tfe_registry_module.go
Outdated
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) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💅 Are these conditions already handled by RegistryModules.Create ? It might simplify things to just delegate this kind of validation to the client.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes they are. I had similar thought as well but I wasn't sure of the validation pattern. I will update this.
tfe/resource_tfe_registry_module.go
Outdated
@@ -59,39 +67,109 @@ func resourceTFERegistryModule() *schema.Resource { | |||
}, | |||
}, | |||
}, | |||
"namespace": { | |||
Type: schema.TypeString, | |||
Computed: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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)
dcdb8d8
to
786db53
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
Description
This PR adds changes that support creating
both private and public registry modules
without using a VCS. It also extends the ability toimport
both private and public registry module by including thenamespace
andregistry-name
in the import id.Remember to:
Update the Change Log
Update the Documentation
Testing plan
Deprecated import format
<ORGANIZATION>/<REGISTRY MODULE NAME>/<REGISTRY MODULE PROVIDER>/<REGISTRY MODULE ID>
works for only private module:#should succeed
#should fail
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
#should succeed
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.