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

lang: Fix crash in lookup function #19161

Merged
merged 2 commits into from
Oct 23, 2018
Merged

lang: Fix crash in lookup function #19161

merged 2 commits into from
Oct 23, 2018

Conversation

radeksimko
Copy link
Member

Previously a config like this:

locals {
  terraform_versions = {
    "terraform-provider-aws" = "0.10+"
  }
}
provider "github" {
  organization = var.organization_name
  token = var.github_token
}

data "github_repositories" "providers" {
  query = "org:${var.organization_name} archived:false aws OR azurerm OR google OR opc OR vsphere OR kubernetes"
}

data "github_repository" "provider" {
  count = length(data.github_repositories.providers.full_names)
  full_name = sort(data.github_repositories.providers.full_names)[count.index]
}

data "template_file" "readme" {
  count = length(data.github_repository.provider.*.ssh_clone_url)

  template = file("./templates/README.md")

  vars = {
    go_version = lookup(local.go_versions, data.github_repository.provider.*.name[count.index], local.default_go_version)
  }
}

caused a crash:

https://gist.github.com/radeksimko/012c83a42c9f6b647912b62b77e4f8ed

switch {
case ty.IsObjectType():
if !args[1].IsKnown() {
return args[2].Type(), nil
Copy link
Contributor

Choose a reason for hiding this comment

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

Doing this check in here does seem right but I don't think returning the "default" value type here is the right fallback for a couple reasons:

  • There might not be a default value (it's optional, and so I think this might cause an out-of-bounds panic)
  • Once we do know the key (at apply time) we might find that it matches one of the attributes in inputMap after all, and so that'll be the type of our result.

With both of those things in mind, I think the best we can do in this scenario is return cty.DynamicPseudoType, nil, which effectively means "I don't know yet what the result type will be". That's an okay response when arguments are unknown: it'll just mean we won't catch a type error here during the plan operation. We'll re-run the function again at apply time when we've learned more information, and thus catch any name error there, as usual for unknown values.

Copy link
Member Author

Choose a reason for hiding this comment

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

That makes sense. I made that change - PTAL.

Copy link
Contributor

@apparentlymart apparentlymart left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@radeksimko radeksimko merged commit 1de7c58 into master Oct 23, 2018
@radeksimko radeksimko deleted the b-lookup-crash-fix branch October 23, 2018 18:19
@ghost
Copy link

ghost commented Apr 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 Apr 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.

3 participants