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

interpolate: Interpolate computed list attributes #2157

Merged
merged 2 commits into from
Aug 27, 2015

Conversation

radeksimko
Copy link
Member

This will solve issues with all list attributes that are computed and currently can't be actually used as lists (i.e. we can't use built-in functions like join()).

The current output coming from valueResourceVar is config.UnknownVariableValue since it's looking for resource.attribute whereas it's present as resource.attribute.# + resource.attribute.0 + resource.attribute.1 etc. in the given map.

Status:

  • Implementation for resources w/out count
  • Implementation for resources w/ count
  • Tests for resources w/out count
  • Tests for resources w/ count

Related: #1999

Test plan

TypeList

resource "aws_route53_zone" "main" {
  name = "mainexample.com"
}

output "new" {
  value = "${join(" ", aws_route53_zone.main.name_servers)}"
}
output "old" {
  value = "${aws_route53_zone.main.name_servers.0} ${aws_route53_zone.main.name_servers.1} ${aws_route53_zone.main.name_servers.2} ${aws_route53_zone.main.name_servers.3}"
}

TypeMap

resource "aws_instance" "sample" {
  ami = "ami-2cd3dc44"
  instance_type = "t2.micro"

  tags {
    Name = "TerraformTest"
    CustomTag = "Yada Yada"
  }
}

output "tags" {
  value = "${aws_instance.sample.tags.Name}"
}
resource "aws_instance" "sample" {
  ami = "ami-2cd3dc44"
  instance_type = "t2.micro"

  # not sure if anyone would want to do this, but it's possible
  tags {
    "0" = "TerraformTest"
    "1" = "Yada Yada"
  }
}

output "tags" {
  value = "${join(",", aws_instance.sample.tags)}"
}

TypeSet

resource "aws_elb" "bar" {
  name = "foobar-terraform-elb"
  availability_zones = ["us-east-1b", "us-east-1c", "us-east-1d"]

  listener {
    instance_port = 8080
    instance_protocol = "http"
    lb_port = 80
    lb_protocol = "http"
  }
}

output "azs" {
  value = "${join(",", aws_elb.bar.availability_zones)}"
}

Impact

$ grep -l -R -E '(TypeSet|TypeList)' ./builtin/providers/* | sort | uniq
./builtin/providers/atlas/resource_artifact.go
./builtin/providers/aws/autoscaling_tags.go
./builtin/providers/aws/provider.go
./builtin/providers/aws/resource_aws_autoscaling_group.go
./builtin/providers/aws/resource_aws_autoscaling_notification.go
./builtin/providers/aws/resource_aws_cloudwatch_metric_alarm.go
./builtin/providers/aws/resource_aws_db_instance.go
./builtin/providers/aws/resource_aws_db_parameter_group.go
./builtin/providers/aws/resource_aws_db_security_group.go
./builtin/providers/aws/resource_aws_db_subnet_group.go
./builtin/providers/aws/resource_aws_dynamodb_table.go
./builtin/providers/aws/resource_aws_ecs_service.go
./builtin/providers/aws/resource_aws_ecs_task_definition.go
./builtin/providers/aws/resource_aws_elasticache_cluster.go
./builtin/providers/aws/resource_aws_elasticache_parameter_group.go
./builtin/providers/aws/resource_aws_elasticache_security_group.go
./builtin/providers/aws/resource_aws_elasticache_subnet_group.go
./builtin/providers/aws/resource_aws_elb.go
./builtin/providers/aws/resource_aws_iam_group_membership.go
./builtin/providers/aws/resource_aws_iam_instance_profile.go
./builtin/providers/aws/resource_aws_iam_policy_attachment.go
./builtin/providers/aws/resource_aws_instance.go
./builtin/providers/aws/resource_aws_launch_configuration.go
./builtin/providers/aws/resource_aws_network_acl.go
./builtin/providers/aws/resource_aws_network_interface.go
./builtin/providers/aws/resource_aws_proxy_protocol_policy.go
./builtin/providers/aws/resource_aws_route53_delegation_set.go
./builtin/providers/aws/resource_aws_route53_record.go
./builtin/providers/aws/resource_aws_route53_zone.go
./builtin/providers/aws/resource_aws_route_table.go
./builtin/providers/aws/resource_aws_s3_bucket.go
./builtin/providers/aws/resource_aws_security_group.go
./builtin/providers/aws/resource_aws_security_group_rule.go
./builtin/providers/aws/resource_aws_vpc_dhcp_options.go
./builtin/providers/aws/resource_aws_vpn_connection.go
./builtin/providers/azure/resource_azure_instance.go
./builtin/providers/azure/resource_azure_local_network.go
./builtin/providers/azure/resource_azure_security_group_rule.go
./builtin/providers/azure/resource_azure_sql_database_server_firewall_rule.go
./builtin/providers/azure/resource_azure_virtual_network.go
./builtin/providers/cloudstack/resource_cloudstack_egress_firewall.go
./builtin/providers/cloudstack/resource_cloudstack_firewall.go
./builtin/providers/cloudstack/resource_cloudstack_network_acl_rule.go
./builtin/providers/cloudstack/resource_cloudstack_port_forward.go
./builtin/providers/consul/resource_consul_keys.go
./builtin/providers/digitalocean/resource_digitalocean_droplet.go
./builtin/providers/docker/resource_docker_container.go
./builtin/providers/google/resource_compute_firewall.go
./builtin/providers/google/resource_compute_instance.go
./builtin/providers/google/resource_compute_instance_template.go
./builtin/providers/google/resource_compute_route.go
./builtin/providers/google/resource_compute_target_pool.go
./builtin/providers/google/resource_dns_managed_zone.go
./builtin/providers/google/resource_dns_record_set.go
./builtin/providers/heroku/resource_heroku_addon.go
./builtin/providers/heroku/resource_heroku_app.go
./builtin/providers/mailgun/resource_mailgun_domain.go
./builtin/providers/openstack/resource_openstack_blockstorage_volume_v1.go
./builtin/providers/openstack/resource_openstack_compute_instance_v2.go
./builtin/providers/openstack/resource_openstack_compute_secgroup_v2.go
./builtin/providers/openstack/resource_openstack_compute_servergroup_v2.go
./builtin/providers/openstack/resource_openstack_fw_policy_v1.go
./builtin/providers/openstack/resource_openstack_lb_pool_v1.go
./builtin/providers/openstack/resource_openstack_networking_subnet_v2.go

@radeksimko radeksimko force-pushed the interpolate-list-attr branch 6 times, most recently from 85fe60d to 5651430 Compare May 31, 2015 13:56
@radeksimko radeksimko changed the title interpolate: Add support for computed TypeList attributes (WIP) interpolate: Add support for computed TypeList attributes May 31, 2015
@radeksimko radeksimko changed the title interpolate: Add support for computed TypeList attributes interpolate: Add support for list attributes May 31, 2015
@radeksimko radeksimko changed the title interpolate: Add support for list attributes interpolate: Interpolate computed list attributes May 31, 2015
@mitchellh
Copy link
Contributor

On a preliminary scan this looks good to me. I'm going to keep scanning to look for anything... but if the rest of the unit tests pass its a pretty good sign.

@phinze can you take a look too? I'd feel better if you checked off on this as well.

@phinze
Copy link
Contributor

phinze commented Jun 9, 2015

@radeksimko this is looking great so far!

This implementation applies to all three of TypeList, TypeSet, and TypeMap, correct? I just want to make sure we clarify that in variable / function naming. I see you picked up on the convention of calling these "multi" variables - which is good. I think there are just a few places you still call them "lists" which confused me for a second.

})
// Zero + zero elements
testInterpolate(t, i, scope, "aws_route53_zone.terra.*.nothing", ast.Variable{
Value: config.InterpSplitDelim, // Not sure if this is desired behaviour?
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm yeah I guess I'd expect an empty list here, but a single InterpSplitDelim indicates ["", ""] right? Another valid return here would be a list of two empty lists [[], []], but we're "flattening" the two sub-lists already it seems like [] is what we should shoot for.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was personally tempted to make it a single empty list as well, but considering the debate in here #1495 (comment) I realise some people may consider that to be way too magic.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think it's worth rabbit holing too much on this, so I'm ultimately fine with it either way, but my vote would still be for a single list, since our "string-hack" lists don't allow for nesting right now.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see your point, I just need to find out how to express an empty list via our "string-hack".

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just need to find out how to express an empty list via our "string-hack".

I've got an idea on that - related to fix for #2240 - finishing up a PR now and I'll highlight you.

@radeksimko
Copy link
Member Author

@phinze Is there any update on the fix for #2240 ?

@radeksimko radeksimko added the waiting-response An issue/pull request is waiting for a response from the community label Jun 13, 2015
@radeksimko
Copy link
Member Author

I reckon this is the related PR: #2504 which should make representation of empty list possible.

@phinze
Copy link
Contributor

phinze commented Jun 26, 2015

@radeksimko yep! finally 😀

If you rebase on top of that and use the StringList type - I believe this might "just work". (And I'm happy to help out if it does not.)

@radeksimko radeksimko removed the waiting-response An issue/pull request is waiting for a response from the community label Jun 26, 2015
@zollie
Copy link

zollie commented Jun 30, 2015

Hey @radeksimko,

Do you plan on updating to StringList? I am trying to render the route53.nameservers list as in your example ...

@radeksimko
Copy link
Member Author

@zollie Thanks for pinging me, I was a bit busy with other things, but I do plan to update it fairly soon. The change may not make it to 0.6.0 though, which is knocking on the door (AFAIK).

I should have some time this week - can't be more specific now, sorry.

@zollie
Copy link

zollie commented Jun 30, 2015

Cool...thanks! Looking forward to it.

@radeksimko radeksimko force-pushed the interpolate-list-attr branch 3 times, most recently from df070d2 to 5eea388 Compare July 3, 2015 12:17
@radeksimko
Copy link
Member Author

All refactoring finished.

@phinze to your question:

This implementation applies to all three of TypeList, TypeSet, and TypeMap, correct?

It applies to TypeList & TypeSet and may eventually apply to TypeMap (if anyone decides to use numbered keys for whatever reason). See examples in Test Plan in my very first comment of this thread.

I have also added a couple of tests for the string_list.

@phinze
Copy link
Contributor

phinze commented Aug 27, 2015

Thanks to @radeksimko for reminding me about this one. LGTM - landing, which I believe should fix #2795!

phinze added a commit that referenced this pull request Aug 27, 2015
interpolate: Interpolate computed list attributes
@phinze phinze merged commit af5dac1 into hashicorp:master Aug 27, 2015
@radeksimko radeksimko deleted the interpolate-list-attr branch August 27, 2015 15:48
@phinze
Copy link
Contributor

phinze commented Aug 27, 2015

p.s. great work on this, @radeksimko! 😀

phinze added a commit that referenced this pull request Jan 26, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs #2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
phinze added a commit that referenced this pull request Jan 26, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs #2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
phinze added a commit that referenced this pull request Jan 26, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs #2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
phinze added a commit that referenced this pull request Jan 26, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs #2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
joshmyers pushed a commit to joshmyers/terraform that referenced this pull request Feb 18, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs hashicorp#2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
bigkraig pushed a commit to bigkraig/terraform that referenced this pull request Mar 1, 2016
References to computed list-ish attributes (set, list, map) were being
improperly resolved as an empty list `[]` during the plan phase (when
the value of the reference is not yet known) instead of as an
UnknownValue.

A "diffs didn't match" failure in an AWS DirectoryServices test led to
this discovery (and this commit fixes the failing test):

https://travis-ci.org/hashicorp/terraform/jobs/104812951

Refs hashicorp#2157 which has the original work to support computed list
attributes at all. This is just a simple tweak to that work.

/cc @radeksimko
@ghost
Copy link

ghost commented May 1, 2020

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.

@ghost ghost locked and limited conversation to collaborators May 1, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants