Navigation Menu

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

configs: fix module for_each call bug #31091

Merged
merged 1 commit into from May 20, 2022

Conversation

kmoe
Copy link
Member

@kmoe kmoe commented May 20, 2022

This fixes a bug introduced in #30639 in which initialising a module will fail if that module contains both a provider block and a module call using for_each.

This bug was introduced in https://github.com/hashicorp/terraform/pull/30639/files#diff-95e614048e1ba4ba7621e22ba67cfe5c8acd386e5540b8c92c267d2fd1b3d31eR31-R55 by removing a variable scoped to a for loop (var nope) and instead overwriting noProviderConfigRange. This meant that the error at https://github.com/hashicorp/terraform/pull/30639/files#diff-95e614048e1ba4ba7621e22ba67cfe5c8acd386e5540b8c92c267d2fd1b3d31eR181 would be triggered if the parent module contained a provider block, not the child.

Fixes #31081

Test case

There are no tests in code for this behaviour. Highly recommend adding them as follow up. I manually tested this using the example below, kindly provided by @knbnnate.

.
├── barren_child
│   └── main.tf
├── grandchild
│   └── main.tf
└── main.tf

main.tf

provider "azurerm" {
  features {
  }
}
terraform {
  backend "local" {
  }
  required_providers {
    azurerm = {
      source  = "hashicorp/azurerm"
      version = "~> 2.99"
    }
  }
}


module "child" {
  source = "./barren_child"
}

barren_child/main.tf

provider "azurerm" {
  alias           = "other"
  subscription_id = var.some_other_sub
  features {
  }
}

module "grandchild" {
  for_each = toset(["bar","baz"])
  source = "../grandchild"
}

grandchild/main.tf

variable "text" {
  type = string
  default = "foo"
  description = "stuff"
}
resource "local_file" "foo" {
  filename = "/dev/null"
  content = jsonencode(var.text)
}

This fixes a bug introduced in 1879a39 in which initialising a module will fail
if that module contains both a provider block and a module call using for_each.
@kmoe kmoe added the 1.2-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged label May 20, 2022
@kmoe kmoe requested a review from a team May 20, 2022 10:44
Copy link
Member

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

This looks good to me! I also tested it with some reconstructions of some test cases I was using when I originally worked on the other PR, just to satisfy myself that I remembered enough about what was going on here to understand why this change makes sense.

I have also started preparing a regression test case for this, but it ended up more annoying to integrate than I expected due to it being a concern that ends up crossing packages. If I get that working well I'll send it in as a separate PR so that we can get this landed sooner.

Thanks for diving in to this!

@kmoe kmoe merged commit 5417975 into main May 20, 2022
@kmoe kmoe deleted the kmoe/module-is-incompatible-with-count branch May 20, 2022 15:58
@github-actions
Copy link

Reminder for the merging maintainer: if this is a user-visible change, please update the changelog on the appropriate release branch.

@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jun 20, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
1.2-backport If you add this label to a PR before merging, backport-assistant will open a new PR once merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1.2.0 errors out with "Module is incompatible with count, for_each, and depends_on"
2 participants