Skip to content

Remove aws provider config from module #35

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

Merged
merged 1 commit into from
Mar 1, 2022
Merged

Remove aws provider config from module #35

merged 1 commit into from
Mar 1, 2022

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