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

[WIP] Add labels support to node_pools #205

Closed
wants to merge 1 commit into from

Conversation

selmanj
Copy link
Contributor

@selmanj selmanj commented Jul 17, 2017

Posting this early as WIP as I added some testing helpers. Hoping for early feedback. Information about the test changes below:

ConfigBuilder is intended to build terraform config strings for tests;
the hope is that tests can be made more legible by directly setting the
parameters the test is interested in and then testing rather than having
the values embedded in an opaque string defined elsewhere.

Some wins:

  • The test is somewhat more readable; you can see that the three values we are testing for are directly set in the test. Those values are then later checked.
  • Providing pregenerated builders with the bare minimum necessary to build a resource allows the tests to focus on specifics rather than all the ceremony needed to build (for example, the cluster is not dealt with in the test at all as its not important).

Some losses:

  • The test runs extremely slow due to how long it takes to provision GKE node pools; because of this there's only one test, and the test itself tests multiple things! I think this builder pattern would be more powerful with a lot more tests as then there's a lot of extra details we don't care about that the builder takes care of for us.

ConfigBuilder is intended to build terraform config strings for tests;
the hope is that tests can be made more legible by directly setting the
parameters the test is interested in and then testing rather than having
the values embedded in an opaque string defined elsewhere.
@rileykarson
Copy link
Collaborator

One of my concerns about using builders like this is that it means we are no longer testing (easy to read/copy) .tf configs.

For me at least, when trying to debug issues, I'll often take configs from the tests, since they are valid configurations that I can copy/paste and that the GCP servers will accept, and run those interactively with the Terraform binary and a development copy of the google plugin, looking at what happens when I make small changes, or at the output of plan/refresh.

We have a corpus of examples in our documentation, but we don't test those; we can only kind of hope that they work (This is something that would be nice to fix in the future). The configurations in our tests are a pool of known-good examples covering how to use each resource.

@selmanj
Copy link
Contributor Author

selmanj commented Jul 17, 2017

@rileykarson You're correct that the full configs are harder to view now (you have to manually print them out via String(). However I'd be surprised if anyone other than terraform developers use the configs embedded in tests as examples of how to use the resource (and if this is the case, well, why is this more convenient than looking at the examples in the docs?).

One thing that might help is that, upon failure, the test prints out the config used so that it can be debugged quicker - perhaps we could adjust the test runner to do that. Would that make the builder more agreeable?

BTW I'm hoping to have a better example of why it's useful in this PR; currently working through some strange API issues. Will update when I have it.

@selmanj
Copy link
Contributor Author

selmanj commented Aug 1, 2017

This change ended up being pretty controversial and I haven't heard much feedback. Rather than blocking the node pool work on this, I'll split it out into two separate PRs (one for builder, one for the node pool change).

@selmanj
Copy link
Contributor Author

selmanj commented Aug 7, 2017

Labels have been added as part of #184.

@selmanj selmanj closed this Aug 7, 2017
@selmanj selmanj deleted the add_labels_to_node_pools branch August 7, 2017 18:31
luis-silva pushed a commit to luis-silva/terraform-provider-google that referenced this pull request May 21, 2019
<!-- This change is generated by MagicModules. -->
/cc @danawillow
@ghost
Copy link

ghost commented Mar 31, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 [email protected]. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 31, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants