hashicorp / vscode-terraform

HashiCorp Terraform VSCode extension
https://marketplace.visualstudio.com/items?itemName=HashiCorp.terraform
Mozilla Public License 2.0
929 stars 180 forks source link

Module inputs rendered as invalid for Registry module #1582

Closed radeksimko closed 1 year ago

radeksimko commented 1 year ago

It looks like there is other stuff broken as well, .tf files that were fine are now showing as invalid. Reverting to 2.27.2 fixes all the issues. This is with the Enhanced Validation switched on (which I also have switched on when I test 2.27.2).

image

Originally posted by @scott-doyland-burrows in https://github.com/hashicorp/vscode-terraform/issues/1579#issuecomment-1753175629

radeksimko commented 1 year ago

I can reproduce this bug and I suspect this is caused by our own handling to default = null, in this particular example here: https://github.com/cloudposse/terraform-null-label/blob/488ab91e34a24a86957e397d9f7262ec5925586a/variables.tf#L144

This may also be related to https://github.com/hashicorp/terraform-ls/issues/894

The fix would likely involve these LOC https://github.com/hashicorp/terraform-schema/blob/e75102034ca47fc3f47eaa08a8d7ee09fc8adb4b/earlydecoder/load_module.go#L224-L228

Interestingly, none of the variables in the linked module seem to declare nullable = true so I'm not sure what's the context - historically or currently - behind that "feature flag". It may be implied as true in the recent versions. We'll have to look into that.

radeksimko commented 1 year ago

While I still believe this is related to nullable variables, we actually handle those within the language server alone correctly, which can be demonstrated by running terraform init and seeing all of those variables show up as optional:

Screenshot 2023-10-10 at 07 05 41

It appears that the Registry API, which is where we obtain the data if we don't have other sources (like locally installed module), doesn't reflect nullable variables:

curl https://registry.terraform.io/v1/modules/cloudposse/label/null/0.25.0 | jq .root.inputs
[
  {
    "name": "environment",
    "type": "string",
    "description": "ID element. Usually used for region e.g. 'uw2', 'us-west-2', OR role 'prod', 'staging', 'dev', 'UAT'",
    "default": "",
    "required": true
  },
  {
    "name": "delimiter",
    "type": "string",
    "description": "Delimiter to be used between ID elements.\nDefaults to `-` (hyphen). Set to `\"\"` to use no delimiter at all.\n",
    "default": "",
    "required": true
  },
  {
    "name": "attributes",
    "type": "list(string)",
    "description": "ID element. Additional attributes (e.g. `workers` or `cluster`) to add to `id`,\nin the order they appear in the list. New attributes are appended to the\nend of the list. The elements of the list are joined by the `delimiter`\nand treated as a single ID element.\n",
    "default": "[]",
    "required": false
  },
  {
    "name": "labels_as_tags",
    "type": "set(string)",
    "description": "Set of labels (ID elements) to include as tags in the `tags` output.\nDefault is to include all labels.\nTags with empty values will not be included in the `tags` output.\nSet to `[]` to suppress all generated tags.\n**Notes:**\n  The value of the `name` tag, if included, will be the `id`, not the `name`.\n  Unlike other `null-label` inputs, the initial setting of `labels_as_tags` cannot be\n  changed in later chained modules. Attempts to change it will be silently ignored.\n",
    "default": "[\n  \"default\"\n]",
    "required": false
  },
  {
    "name": "tags",
    "type": "map(string)",
    "description": "Additional tags (e.g. `{'BusinessUnit': 'XYZ'}`).\nNeither the tag keys nor the tag values will be modified by this module.\n",
    "default": "{}",
    "required": false
  },
  {
    "name": "additional_tag_map",
    "type": "map(string)",
    "description": "Additional key-value pairs to add to each map in `tags_as_list_of_maps`. Not added to `tags` or `id`.\nThis is for some rare cases where resources want additional configuration of tags\nand therefore take a list of maps with tag key, value, and additional configuration.\n",
    "default": "{}",
    "required": false
  },
  {
    "name": "context",
    "type": "any",
    "description": "Single object for setting entire context at once.\nSee description of individual variables for details.\nLeave string and numeric variables as `null` to use default value.\nIndividual variable settings (non-null) override settings in context object,\nexcept for attributes, tags, and additional_tag_map, which are merged.\n",
    "default": "{\n  \"additional_tag_map\": {},\n  \"attributes\": [],\n  \"delimiter\": null,\n  \"descriptor_formats\": {},\n  \"enabled\": true,\n  \"environment\": null,\n  \"id_length_limit\": null,\n  \"label_key_case\": null,\n  \"label_order\": [],\n  \"label_value_case\": null,\n  \"labels_as_tags\": [\n    \"unset\"\n  ],\n  \"name\": null,\n  \"namespace\": null,\n  \"regex_replace_chars\": null,\n  \"stage\": null,\n  \"tags\": {},\n  \"tenant\": null\n}",
    "required": false
  },
  {
    "name": "label_key_case",
    "type": "string",
    "description": "Controls the letter case of the `tags` keys (label names) for tags generated by this module.\nDoes not affect keys of tags passed in via the `tags` input.\nPossible values: `lower`, `title`, `upper`.\nDefault value: `title`.\n",
    "default": "",
    "required": true
  },
  {
    "name": "namespace",
    "type": "string",
    "description": "ID element. Usually an abbreviation of your organization name, e.g. 'eg' or 'cp', to help ensure generated IDs are globally unique",
    "default": "",
    "required": true
  },
  {
    "name": "stage",
    "type": "string",
    "description": "ID element. Usually used to indicate role, e.g. 'prod', 'staging', 'source', 'build', 'test', 'deploy', 'release'",
    "default": "",
    "required": true
  },
  {
    "name": "name",
    "type": "string",
    "description": "ID element. Usually the component or solution name, e.g. 'app' or 'jenkins'.\nThis is the only ID element not also included as a `tag`.\nThe \"name\" tag is set to the full `id` string. There is no tag with the value of the `name` input.\n",
    "default": "",
    "required": true
  },
  {
    "name": "label_order",
    "type": "list(string)",
    "description": "The order in which the labels (ID elements) appear in the `id`.\nDefaults to [\"namespace\", \"environment\", \"stage\", \"name\", \"attributes\"].\nYou can omit any of the 6 labels (\"tenant\" is the 6th), but at least one must be present.\n",
    "default": "",
    "required": true
  },
  {
    "name": "regex_replace_chars",
    "type": "string",
    "description": "Terraform regular expression (regex) string.\nCharacters matching the regex will be removed from the ID elements.\nIf not set, `\"/[^a-zA-Z0-9-]/\"` is used to remove all characters other than hyphens, letters and digits.\n",
    "default": "",
    "required": true
  },
  {
    "name": "label_value_case",
    "type": "string",
    "description": "Controls the letter case of ID elements (labels) as included in `id`,\nset as tag values, and output by this module individually.\nDoes not affect values of tags passed in via the `tags` input.\nPossible values: `lower`, `title`, `upper` and `none` (no transformation).\nSet this to `title` and set `delimiter` to `\"\"` to yield Pascal Case IDs.\nDefault value: `lower`.\n",
    "default": "",
    "required": true
  },
  {
    "name": "descriptor_formats",
    "type": "any",
    "description": "Describe additional descriptors to be output in the `descriptors` output map.\nMap of maps. Keys are names of descriptors. Values are maps of the form\n`{\n   format = string\n   labels = list(string)\n}`\n(Type is `any` so the map values can later be enhanced to provide additional options.)\n`format` is a Terraform format string to be passed to the `format()` function.\n`labels` is a list of labels, in order, to pass to `format()` function.\nLabel values will be normalized before being passed to `format()` so they will be\nidentical to how they appear in `id`.\nDefault is `{}` (`descriptors` output will be empty).\n",
    "default": "{}",
    "required": false
  },
  {
    "name": "enabled",
    "type": "bool",
    "description": "Set to false to prevent the module from creating any resources",
    "default": "",
    "required": true
  },
  {
    "name": "id_length_limit",
    "type": "number",
    "description": "Limit `id` to this many characters (minimum 6).\nSet to `0` for unlimited length.\nSet to `null` for keep the existing setting, which defaults to `0`.\nDoes not affect `id_full`.\n",
    "default": "",
    "required": true
  },
  {
    "name": "tenant",
    "type": "string",
    "description": "ID element _(Rarely used, not included by default)_. A customer identifier, indicating who this instance of a resource is for",
    "default": "",
    "required": true
  }
]

I will try to reach out to the Registry team internally to see if we can get that fixed.


The best workaround for the time being would be to run terraform init to install the Registry module, or to disable enhanced validation via "terraform.validation.enableEnhancedValidation": false

radeksimko commented 1 year ago

Somewhat good news: It looks like this only affects older modules, i.e. this is a data bug, rather than an implementation bug in the Registry.

I tried re-ingesting the same module by forking it and I can see the affected variables come through correctly as optional:

Screenshot 2023-10-10 at 12 19 18

I am engaging with the Registry team to see if there's any way to re-ingest the module (and possibly other ones affected by the same problem) or otherwise correct the data.

radeksimko commented 1 year ago

After some internal conversations and taking some time to think about the scale of the problem and implications, I came up with a (hopefully) pragmatic solution:

This is currently pending review. We'll post update here when it becomes part of a release.

dbanck commented 1 year ago

A new version 2.28.2 was just released. This fixes the reported bug. The update should appear automatically in VS Code.


In case you experience any different validation related bug, please do let us know through a new issue.

github-actions[bot] commented 1 year ago

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.