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

Migrate to for_each. Get rid of *_num options (breaking change). Fix destructiveness of updates. Add new playground test suite for testing updates. #64

Merged
merged 25 commits into from
Nov 7, 2019

Conversation

cray0000
Copy link
Contributor

@cray0000 cray0000 commented Oct 9, 2019

API changes

  1. *_num options are removed.
  2. Support dynamic entities (for example, projects in projects_iam) only when 1 entity is passed. If you pass 2 or more, the configuration has to be static, meaning that it can't use any of the other resource's fields to get the entity name from (this includes getting the randomly generated hashes through the random_id resource).
  3. The roles can never be dynamic.
  4. Members can only be dynamic in authoritative mode.

Testing configuration update.

In the update test suite I've setup an additional variable roles (defaults to 2) which you can pass as TF_VAR_roles=3 when running the converge command. The fixtures currently support the amount of roles from 1 to 3. But it's trivial to add more, if needed in future.

This way we can reconverge the update test suite with different TF_VAR_roles values and verify. And by doing this multiple times we are basically testing the module configuration update behavior.

@cray0000
Copy link
Contributor Author

cray0000 commented Oct 9, 2019

Found a way to replicate this with the simplest usecase.
Added a testsuite as a playground to try to fix it.

In the base scenario of having 2 projects and 1 role specified with 1 member in authoritative mode. Adding just one additional role will lead to recreation of one iam resource instead of just reusing it.

https://gist.github.com/cray0000/1f75fec0a6d32d2f31f8a4502e2558cd

@aaron-lane aaron-lane added the bug Something isn't working label Oct 9, 2019
…k to 'count' when dynamic). [test/update] Change test to generate project ids statically instead of getting them from the resource outputs
@cray0000 cray0000 changed the title [IN PROGRESS] Fix destructiveness of updates. Add new playground test suite for testing updates. [IN PROGRESS] Migrate to for_each. Fix destructiveness of updates. Add new playground test suite for testing updates. Oct 10, 2019
…ariations of dynamic/static and authoritative/additive
… mode only for one entity. Support dynamic members only in authoritative mode. [test/update] Update tests to properly work the first time. Should pass on CI now.
…thoritative tests accordingly. Add ability to specify amount of roles to test in the 'update' test suite. [ci] Test how the module behaves on updates (of projects_iam module)
@cray0000 cray0000 changed the title [IN PROGRESS] Migrate to for_each. Fix destructiveness of updates. Add new playground test suite for testing updates. Migrate to for_each. Get rid of *_num options. Fix destructiveness of updates. Add new playground test suite for testing updates. Oct 17, 2019
@cray0000 cray0000 changed the title Migrate to for_each. Get rid of *_num options. Fix destructiveness of updates. Add new playground test suite for testing updates. Migrate to for_each. Get rid of *_num options (breaking change). Fix destructiveness of updates. Add new playground test suite for testing updates. Oct 17, 2019
@cray0000 cray0000 requested a review from aaron-lane October 17, 2019 11:39
…[ci] Test configuration update for the whole authoritavite and additive suites. [test][update] rename update test suite to static-and-dynamic. Refactor.
@aaron-lane aaron-lane self-assigned this Oct 21, 2019
Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks @cray0000!

A few changes requested. In addition, we need an upgrade guide similar to the existing ones under docs since we are breaking the existing interface.

README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
modules/helper/main.tf Outdated Show resolved Hide resolved
test/fixtures/authoritative/outputs.tf Outdated Show resolved Hide resolved
test/integration/additive/controls/additive.rb Outdated Show resolved Hide resolved
test/integration/additive/controls/additive.rb Outdated Show resolved Hide resolved
@Lirt
Copy link

Lirt commented Oct 23, 2019

Hey @cray0000 can I help you with review or testing?
I have mutliple examples with the issue #11, so if you need testing on production, I can do it.

…adme] fix typos. [migration] Add migration guide from 3.0 to 4.0
@cray0000 cray0000 requested a review from aaron-lane October 23, 2019 16:49
@aaron-lane aaron-lane added the enhancement New feature or request label Oct 24, 2019
docs/upgrading_to_iam_4.0.md Outdated Show resolved Hide resolved
docs/upgrading_to_iam_4.0.md Show resolved Hide resolved
…ynamic] Refactor procedural generation of tests.
@morgante
Copy link
Contributor

@cray0000 Can you make sure tests pass?

Copy link
Contributor

@aaron-lane aaron-lane left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. Just one request.

test/integration/helper/libraries/bindings.rb Outdated Show resolved Hide resolved
@cray0000 cray0000 requested a review from aaron-lane November 6, 2019 15:26
@aaron-lane aaron-lane merged commit 369f5d5 into terraform-google-modules:master Nov 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working enhancement New feature or request
Projects
None yet
4 participants