minamijoyo / tfupdate

Update version constraints in your Terraform configurations
MIT License
539 stars 23 forks source link

Non-HashiCorp provider updates failing #99

Closed nikolay closed 9 months ago

nikolay commented 9 months ago

For example,

$ tfupdate provider cloudflare/cloudflare -v 4.15.0 -r .

completes with success but did not upgrade anything.

minamijoyo commented 9 months ago

Hi @nikolay, If I recall correctly, the current implementation doesn't care about the namespace. Of course, this means there are other problems, but if it doesn't matter to you, you can use cloudflare instead of cloudflare/cloudflare.

nikolay commented 9 months ago

@minamijoyo There are multiple providers called mysql and could exist in one project, for example, so namespace could be optional but must be supported.

minamijoyo commented 9 months ago

Yes, it is a missing feature.

While the current implementation for updating providers still has been naively updating files as they are read, the internal architecture has recently been changed to load the root module first, and then update files. Since the module context already recognizes the required providers by their fully qualified names, it may be relatively easy to support namespaces now.

minamijoyo commented 9 months ago

Memo: The current implementation of tfupdate provider <PROVIDER_NAME> uses the <PROVIDER_NAME> argument as a key for the required_providers block. To support namespaces, we also need to check the source attribute. The most important point is that the key can be almost any string but cannot contain /. Thus, if the argument contains /, we can assume that the user intends to use namespaces and check the source attribute. To maintain backward compatibility of tfupdate, when namespaces are omitted, hashicorp/ should not be implicitly assumed.

This is valid:

terraform {
  required_providers {
    petoju = {
      source  = "petoju/mysql"
      version = "3.0.42"
    }

    winebarrel = {
      source  = "winebarrel/mysql"
      version = "1.10.5"
    }
  }
}

The followings are invalid:

$ terraform -v
Terraform v1.5.7
on darwin_arm64
+ provider registry.terraform.io/petoju/mysql v3.0.42
+ provider registry.terraform.io/winebarrel/mysql v1.10.5

$ cat main.tf
terraform {
  required_providers {
    "petoju/mysql" = {
      source  = "petoju/mysql"
      version = "3.0.42"
    }

    winebarrel = {
      source  = "winebarrel/mysql"
      version = "1.10.5"
    }
  }
}

$ terraform validate
╷
│ Error: Invalid argument name
│
│   on main.tf line 3, in terraform:
│    3:     "petoju/mysql" = {
│
│ Argument names must not be quoted.
╵

$ cat main.tf
terraform {
  required_providers {
    petoju/mysql = {
      source  = "petoju/mysql"
      version = "3.0.42"
    }

    winebarrel = {
      source  = "winebarrel/mysql"
      version = "1.10.5"
    }
  }
}

$ terraform validate
╷
│ Error: Argument or block definition required
│
│   on main.tf line 3, in terraform:
│    3:     petoju/mysql = {
│
│ An argument or block definition is required here. To set an argument, use the equals sign "=" to
│ introduce the argument value.
╵
nikolay commented 9 months ago

I do agree, but you ignore the use case of a monorepo, which is one of the best practices of HashiCorp for multiple environments. So, when I run your tool in a monorepo, there could be multiple mysql aliases.

minamijoyo commented 9 months ago

While the above memo is design research and has not been implemented yet, when the tfupdate provider <PROVIDER_NAME> argument contains /, matching it to the source attribute should also work in the mono repo case.

For example, given the following directories:

dir1/main.tf

terraform {
  required_providers {
    mysql = {
      source  = "petoju/mysql"
      version = "3.0.42"
    }
  }
}

dir2/main.tf

terraform {
  required_providers {
    mysql = {
      source  = "winebarrel/mysql"
      version = "1.10.5"
    }
  }
}

Running tfupdate provider with petoju/mysql should update only dir1, but not dir2, right?

$ tfupdate provider petoju/mysql -v 3.0.43 -r .

Please let me know if my understanding differs from your expectations.

nikolay commented 9 months ago

Yes, dir1 only.

minamijoyo commented 9 months ago

@nikolay Fixed in v0.8.0 🚀