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

Restore AWS required version #180

Closed
jasondamour opened this issue Mar 19, 2024 · 13 comments
Closed

Restore AWS required version #180

jasondamour opened this issue Mar 19, 2024 · 13 comments

Comments

@jasondamour
Copy link
Contributor

jasondamour commented Mar 19, 2024

Description

#178 removed the required_providers block, which now causes the following warning in my env:

│ Warning: Reference to undefined provider
│ 
│   on aws_ecs_cluster.tf line 16, in module "ecs_cluster":
│   16:     aws = aws.us_east_2
│ 
│ There is no explicit declaration for local provider name "aws" in module.ecs_cluster, so Terraform is assuming you mean to pass a
│ configuration for "hashicorp/aws".
│ 
│ If you also control the child module, add a required_providers entry named "aws" with the source address "hashicorp/aws".

Versions

  • Module version [Required]:
    5.10.0

  • Terraform version:
    Terraform v1.7.4

  • Provider version(s):
    + provider registry.terraform.io/hashicorp/aws v5.41.0

Reproduction Code [Required]

module "ecs_cluster" {
  # https://github.com/terraform-aws-modules/terraform-aws-ecs
  source  = "terraform-aws-modules/ecs/aws"
  version = "~> 5.0"

  providers = {
    aws = aws.us_east_2
  }

  cluster_name = "us-east-2-fargate-cluster"
}

Expected behavior

No warning

Actual behavior

Warning printed.

Additional

Tagging @bryantbiggs since your PR removed the block, can you shed some light on that?

@bryantbiggs
Copy link
Member

the AWS provider version is not required because there aren't any direct AWS resources - there are only modules. if you look at the underlying module definitions, these have AWS resources and therefore a required AWS provider version is supplied there

@jasondamour
Copy link
Contributor Author

Could this module still include a versionless requirement? That way its clear it needs the AWS provider, but doesn't have its own version requirement. That way we can still override the provider without warnings:

  required_providers {
    aws = {
      source  = "hashicorp/aws"
    }

@bryantbiggs
Copy link
Member

I'm curious - what are you doing that you need to pass an aliased provider to the root module?

@jasondamour
Copy link
Contributor Author

Should be pretty obvious from my reproduction code, I have different aliases for different regions

@bryantbiggs
Copy link
Member

thats generally a poor practice to follow, putting all/multiple regions into one statefile. if any one region experiences a disruption, you are blocked on making changes. it also means your blast radius is quite high for a single statefile

@jasondamour
Copy link
Contributor Author

jasondamour commented Mar 19, 2024

Nonetheless, it is valid Terraform code. For this module, its reasonable to state that it requires the AWS provider, no?

edit: For what its worth, I also use aliases for multiple separate AWS accounts in the same workspace.

@jasondamour
Copy link
Contributor Author

Its a fairly common practice; here you can see all of the other modules in this github org using the provider.

https://github.com/search?q=org%3Aterraform-aws-modules+required_providers+path%3Aversions.tf&type=code

I'll open a PR for it

@bryantbiggs
Copy link
Member

yes, we have the required provider versions where a provider is used (where a resource is used). There are ZERO resources in the root module of this project, there are simply two sub-modules wrapped by the root module - each sub-module has its own requirements defined.

see - your PR failed, because its not used
image

@jasondamour
Copy link
Contributor Author

jasondamour commented Mar 19, 2024

I think I disagree with this linting rule. Isn't the provider is being used implicitly? To hide this linting error, I could add the following to every child module block in main.tf:

providers = {
  aws = aws
}

However I think that adds needless bloat lines. Otherwise how do you suggest passing a provider alias, if I have two AWS accounts (i.e. aws.account_a and aws.account_b)? I'm open to suggestions.

@antonbabenko
Copy link
Member

I like #181 (missing version requirements there), and I think we should have required_providers like @jasondamour tells.

We should always have it there as long as resources are being created by either the root or by the submodules that are being called from the root. Having the required_providers block indicates that the root module expects to have the hashicorp/aws provider (the original one, and not one of its forks published on the registry) of a specific minimum version available.

Technically, the resources belong to the root module even though the configuration resides in the submodules it calls. Root modules and submodules may have different version requirements.

@jasondamour
Copy link
Contributor Author

Hi @antonbabenko @bryantbiggs, please review and merge #181

I left out the version per @bryantbiggs suggestion, since the version does come from the child modules and it doesn't need to be duplicated

Copy link

This issue has been automatically marked as stale because it has been open 30 days
with no activity. Remove stale label or comment or this issue will be closed in 10 days

Copy link

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 have found a problem that seems similar to this, 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 May 24, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants