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

add identifier to outputs #495

Closed
shilikok opened this issue May 31, 2023 · 10 comments · Fixed by #489
Closed

add identifier to outputs #495

shilikok opened this issue May 31, 2023 · 10 comments · Fixed by #489

Comments

@shilikok
Copy link

Is your request related to a new offering from AWS?

Is this functionality available in the AWS provider for Terraform? See CHANGELOG.md, too.

  • Yes ✅: please list the AWS provider version which introduced this functionality
    aws provider: 5.0.0

Is your request related to a problem? Please describe.

id is no longer the AWS database identifier - id is now the dbi-resource-id. Refer to identifier instead of id to use the database's identifier

Describe the solution you'd like.

add to module outputs db identifier value

Describe alternatives you've considered.

Additional context

@daroga0002
Copy link
Contributor

@bryantbiggs this change will be not breaking one.
identifier fields exists from very long time (I checked few random versions of provider in major versions 2, 3, 4).

It is passed as input to resource (so it can be also outputed) here:
https://github.com/terraform-aws-modules/terraform-aws-rds/blob/03a74f20b3154186546cf3146338c19cfd08e10b/modules/db_instance/main.tf#LL34C28-L34C36

I have created PR for this

@bryantbiggs
Copy link
Member

while the attribute may have always been there, the value for id has changed between < v4.x & > v5.x

so while simply adding the identifier output seems harmless, I don't think that is providing value without addressing this underlying change

@daroga0002
Copy link
Contributor

daroga0002 commented Jun 1, 2023

Module in single instance scenario is working correctly under v5 provider. Issue is only with local replication instance scenario where you need pass identifier which under v4 provider was equal to id. As now it has changed there is no way to take name of database which can be used by replicate_source_db input to module.

After adding this output in module it will become working correctly with v5 provider in replication scenario (for replicated scenario).

No other changes is required in module to address v5 compatibility as current id output seems be not used anywhere else.

@daroga0002
Copy link
Contributor

daroga0002 commented Jun 1, 2023

@shilikok FYI

I am currently addresing this by making workaround by extracting from instance ARN last element which represent identifier of database

module "database_replica_rds" {
  count   = local.create_database_replica_rds ? 1 : 0
  source  = "terraform-aws-modules/rds/aws"
  version = "5.9.0"

  # Source database. For cross-region use db_instance_arn
  replicate_source_db        = element(split(":", module.database_rds[0].db_instance_arn), length(split(":", module.database_rds[0].db_instance_arn))-1)
  .....
}

@AyodejiO
Copy link

AyodejiO commented Jun 7, 2023

I just had the same issue where I had to assign the replicate_source_db to the local.identifier variable used to create the master DB. You should prolly use that to remove the complex code you have @daroga0002

@daroga0002
Copy link
Contributor

I just had the same issue where I had to assign the replicate_source_db to the local.identifier variable used to create the master DB. You should prolly use that to remove the complex code you have @daroga0002

It will be not working when you use instance_use_identifier_prefix variable.

What is more it will require additional manual dependency establish between main and replica module (when you use outputs from one module dependency is done automaticlly)

@daroga0002
Copy link
Contributor

@bryantbiggs @antonbabenko
I really dont understand what really problem is to add this output to module. It will not brake any compatibility with any currenrtly supported terraform and provider versions. Instead it will allow to use aws provider v5.

@jwoytek
Copy link

jwoytek commented Jun 9, 2023

I have environments that are currently broken when using the newer version of the Terraform AWS provider that includes the PR to change the id attribute. I was filing a bug on this here yesterday as I pulled on that thread because I didn't find this earlier one.

I don't know that I have a strong opinion either way, but it would be nice for a fix to get in so that I can use the newer provider versions. The PR I was about to commit simply modified the db_instance_id output to return the identifier attribute, which would follow what the original intention of that attribute was in this module. db_instance_resource_id already exists to get the resource ID, which is what aws_db_instance now returns as both resource_id and id.

I can see how it might make sense to match the new mapping for id at the provider level and add identifier as a new attribute, too. This would require current users to update their code to use identifier in place of db_instance_id.

If I could vote, I suppose I might vote for the former, because it is less work and anyone doing a provider upgrade will be able to continue to use the module without further issue. I am not sure if it would be backwards-compatible with older supported versions of the provider, but it seems like it would, as identifier has been around for a while.

jwoytek added a commit to jwoytek/terraform-aws-rds that referenced this issue Jun 20, 2023
aws_db_instance was updated by hashicorp/terraform-provider-aws#31232
such that the `id` attribute now tracks the resource ID, and the
identifier is now returned in a new attribute named `identifier`.

The resource ID is already available in `instance_resource_id`. This
fix allows existing users to continue using this module with no
changes and preserves the original function of the `id` attribute
in this module.

Fixes terraform-aws-modules#495.
@antonbabenko
Copy link
Member

This issue has been resolved in version 6.0.0 🎉

@github-actions
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 Jul 26, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
6 participants