Skip to content

Conversation

@Roberdvs
Copy link
Contributor

@Roberdvs Roberdvs commented Feb 25, 2022

Hi!

I came across this module and while I was trying it out I found a problem with our current Terraform configuration because the aws provider is specified inside the module, so it prevents from configuring the provider in some other way in the parent module (like assuming a role).

I believe it would be a best practice to delegate the provider configuration to the parent module that calls this module and removing also the region and default_tags variables, let me know what you think!

Example usage that wasn't possible before:

provider "aws" {
  region = "eu-west-1"

  assume_role {
    role_arn = "arn:aws:iam::123456789:role/some-role"
  }

  default_tags {
    tags = {
	  project = "terraform-aws-organization-and-sso"
      management = "terraform"
    }
  }
}

module "aws_organizations_and_sso" {
	source  = "chris-qa-org/organzation-and-sso/aws"
	
	.
	.
}

@Stretch96
Copy link
Member

Hi @Roberdvs,

Yep, you're completely right - I shouldn't have put the provider config in there 🤦‍♂️ ...

I tested it out, and fortunately it has no bad side effects when removing the provider block which it claims in the docs (https://www.terraform.io/language/modules/develop/providers):

As a consequence, you must ensure that all resources that belong to a particular provider configuration are destroyed before you can remove that provider configuration's block from your configuration. If Terraform finds a resource instance tracked in the state whose provider configuration block is no longer available then it will return an error during planning, prompting you to reintroduce the provider configuration.

I'll run the tests, merge, and version bump

Thanks for noticing and opening the PR 👍

@Stretch96 Stretch96 merged commit 6dd8af0 into chris-qa-org:main Mar 1, 2022
@Roberdvs
Copy link
Contributor Author

Roberdvs commented Mar 2, 2022

Thank you for accepting the PR!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants