-
Notifications
You must be signed in to change notification settings - Fork 6
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
fix: allow this module to be used with a count
argument
#47
base: main
Are you sure you want to change the base?
Conversation
931b76e
to
0c56d28
Compare
examples/deploy/outputs.tf
Outdated
@@ -0,0 +1,4 @@ | |||
output "nutanix_outputs" { | |||
description = "Outputs of the Nutanix module" | |||
value = module.nutanix |
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.
The README example of outputs will also need to be revised
count
argument
…le module directory
0c56d28
to
d2aa3bd
Compare
@@ -52,6 +52,12 @@ To accommodate deployment requirements, this module will create: | |||
|
|||
You'll need [Terraform installed](https://developer.hashicorp.com/terraform/install) and an [Equinix Metal account](https://deploy.equinix.com/developers/docs/metal/identity-access-management/users/) with an [API key](https://deploy.equinix.com/developers/docs/metal/identity-access-management/api-keys/). | |||
|
|||
The terraform configuration for deploying a proof-of-concept Nutanix cluster is in `examples/deploy`. Start by changing to that directory: |
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.
This sentence reads like "the princess is in another castle."
We could move the root .tf files into a module directory and state that this is a reusable module (link to deploy, link to registry for this module, link to reusable TF module documentation) and state that examples of that module are in examples, similar to the Fabric and NE (upcoming) TF Modules.
Doing so, the module and the example would inherit some of this README.md. What remains in the root would be significantly lighter, essentially, the princess is in another castle.
🍄 🏰 🎆
This tips my hat to how I'm currently thinking about equinix-labs/terraform-equinix-labs#47
(^ just for consideration - not review blocking)
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 the dissonance of pointing the user at a different directory in order to run the thing is another indication that this sort of indirection--there's a module, but to actually run it you need to use the wrapper over here / you ran this config, but actually the interesting part was that config underneath--isn't desirable in our example/proof-of-concept modules. Tucking the actual module code in a subdirectory still leaves the user digging through layers to find the information they really want, and the only reason we really need to do that is do allow the module to be dynamically enabled in the Labs repo.
I converted this to a draft for now since it's not clear if the work done here runs counter to the purpose of our proof-of-concept modules. Once we have consensus on how the modules we deliver should be structured we can mark this as ready for review again. |
@@ -154,7 +154,7 @@ resource "null_resource" "wait_for_firstboot" { | |||
} | |||
|
|||
provisioner "remote-exec" { | |||
script = "scripts/firstboot-check.sh" | |||
script = "${path.module}/scripts/firstboot-check.sh" |
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.
Note: this is present in #73
We can't include this module in equinix-labs/terraform-equinix-labs if it includes explicit provider configuration because equinix-labs/terraform-equinix-labs uses a
count
for each of its submodules to enable users to opt in to any combination of those submodules. This PR removes the explicit provider configuration from the module itself and adds an example inexamples/deploy
that configures the provider and can be used to run the now-reusable module. This will require updating the README to direct users to run theexamples/deploy
module; I haven't done that in this PR because #44 is still in flight.