equinix / terraform-equinix-metal-openstack

OpenStack Cloud on Equinix Metal
https://registry.terraform.io/modules/equinix/openstack/metal/latest
Apache License 2.0
13 stars 12 forks source link

Fix tests and don't offer a specific remote backend #48

Closed displague closed 3 years ago

displague commented 3 years ago

Fixes #45 Fixes #47

The main.tf file was converted to backends.hcl format (https://www.terraform.io/docs/backends/types/remote.html#using-cli-input) and moved into a BACKEND_FILE secret in GH Actions

displague commented 3 years ago

@jmarhee What is the null_resource.controller-openstack dependencies that plan is missing here? Was this renamed or is it perhaps in a file that is not in the repository? I can't find references to that resource in the old tagged revisions either.

I do see a few other null_resource.controller* resources, perhaps this reference should be using one of those?

displague commented 3 years ago

The terraform plan on this PR is not finishing

We also seem to be missing ExternalNetwork.sh (https://github.com/equinix/terraform-metal-openstack/issues/37). Is ExternalNetwork.tf necessary (it still had packet_device in it, maybe it was locally deleted in your last test instead of removed from git)?

I got a little further locally by deleting that file and removing the NovaConsole depends_on = [null_resource.controller-openstack] line. I don't know what I might be missing by doing so.

My provision eventually failed because I didn't create the metal-key file with the expected params. I've created #49 as a way to prevent that in the future.

jmarhee commented 3 years ago

The ExternalNetworks.tf was one of the files suffixed with .off, so I didn't catch that a resource it named wasn't present. We can remove this, and reimplement later if necessary.

The resource in NovaConsole should read controller-nova rather than openstack-- this file was also suffixed .off-- updating that resource name should suffice.

displague commented 3 years ago

@jmarhee I've included changes to remove those files.

The plan tests are currently failing because this project is not configured with an EM API token for testing. I thought it might pull this from state using the TF_API token, but that is not the case (perhaps because I renamed the variable, or perhaps because variables are not pulled from backends).

We'll need a service token of sorts, a user token from a user account dedicated to this work that we can configure here. This will need to be defined as METAL_AUTH_TOKEN in the secrets.

I do think we'll need to address #49 too, but we can address that in a separate issue once we see that the plan and apply is working (which it does from my local testing of this branch).

displague commented 3 years ago

If this project can get by with a project token, we could try this: https://github.com/displague/metal-actions-example

It would generate a project id/name/token/ssh key for our testing and destroy it when done. https://github.com/displague/metal-project-action#output

We could use the same projectName on each build so we don't get excessive build failure devices. Instead we will end up with tests getting stuck until the known projectName is manually deleted. The sweeper could run first (if it is taught how to delete projects by name rather than id alone).

displague commented 3 years ago

I've opened another ticket to track the need for adding secrets: https://github.com/equinix/terraform-metal-openstack/issues/50.

If you are comfortable with this PR, @jmarhee, maybe we merge this now (it gets TF planning and applying again) and we'll address the test secrets next.

jmarhee commented 3 years ago

Looks good. I think we can proceed.