fix: Work around 'unreliable' input data for Registry modules (#1456)

* fix: Work around 'unreliable' input data for Registry modules * Rename existing mock data variables * add tests
hashicorp · Oct 12, 2023 · fadfca2 · fadfca2
1 parent 6603965
commit fadfca2
Show file tree

Hide file tree

Showing 6 changed files with 361 additions and 18 deletions.
diff --git a/internal/registry/module.go b/internal/registry/module.go
@@ -11,6 +11,7 @@ import (
 	"net/http"
 	"net/http/httptrace"
 	"sort"
+	"time"
 
 	"github.com/hashicorp/go-version"
 	tfaddr "github.com/hashicorp/terraform-registry-address"
@@ -21,8 +22,9 @@ import (
 )
 
 type ModuleResponse struct {
-	Version string     `json:"version"`
-	Root    ModuleRoot `json:"root"`
+	Version     string     `json:"version"`
+	PublishedAt time.Time  `json:"published_at"`
+	Root        ModuleRoot `json:"root"`
 }
 
 type ModuleRoot struct {

diff --git a/internal/registry/module_test.go b/internal/registry/module_test.go
@@ -47,7 +47,8 @@ func TestGetModuleData(t *testing.T) {
 		t.Fatal(err)
 	}
 	expectedData := &ModuleResponse{
-		Version: "0.0.8",
+		Version:     "0.0.8",
+		PublishedAt: time.Date(2021, time.August, 5, 0, 26, 33, 501756000, time.UTC),
 		Root: ModuleRoot{
 			Inputs: []Input{
 				{

diff --git a/internal/terraform/module/module_ops.go b/internal/terraform/module/module_ops.go
@@ -1063,10 +1063,11 @@ func GetModuleDataFromRegistry(ctx context.Context, regClient registry.Client, m
 
 		inputs := make([]tfregistry.Input, len(metaData.Root.Inputs))
 		for i, input := range metaData.Root.Inputs {
+			isRequired := isRegistryModuleInputRequired(metaData.PublishedAt, input)
 			inputs[i] = tfregistry.Input{
 				Name:        input.Name,
 				Description: lang.Markdown(input.Description),
-				Required:    input.Required,
+				Required:    isRequired,
 			}
 
 			inputType := cty.DynamicPseudoType
@@ -1123,3 +1124,21 @@ func GetModuleDataFromRegistry(ctx context.Context, regClient registry.Client, m
 
 	return errs.ErrorOrNil()
 }
+
+// isRegistryModuleInputRequired checks whether the module input is required.
+// It reflects the fact that modules ingested into the Registry
+// may have used `default = null` (implying optional variable) which
+// the Registry wasn't able to recognise until ~ 19th August 2022.
+func isRegistryModuleInputRequired(publishTime time.Time, input registry.Input) bool {
+	fixTime := time.Date(2022, time.August, 20, 0, 0, 0, 0, time.UTC)
+	// Modules published after the date have "nullable" inputs
+	// (default = null) ingested as Required=false and Default="null".
+	//
+	// The same inputs ingested prior to the date make it impossible
+	// to distinguish variable with `default = null` and missing default.
+	if input.Required && input.Default == "" && publishTime.Before(fixTime) {
+		// To avoid false diagnostics, we safely assume the input is optional
+		return false
+	}
+	return input.Required
+}
diff --git a/internal/terraform/module/module_ops_mock_responses.go b/internal/terraform/module/module_ops_mock_responses.go
@@ -3,8 +3,15 @@
 
 package module
 
-// moduleVersionsMockResponse represents response from https://registry.terraform.io/v1/modules/puppetlabs/deployment/ec/versions
-var moduleVersionsMockResponse = `{
+import (
+	"github.com/hashicorp/go-version"
+	"github.com/hashicorp/hcl-lang/lang"
+	tfregistry "github.com/hashicorp/terraform-schema/registry"
+	"github.com/zclconf/go-cty/cty"
+)
+
+// puppetModuleVersionsMockResponse represents response from https://registry.terraform.io/v1/modules/puppetlabs/deployment/ec/versions
+var puppetModuleVersionsMockResponse = `{
   "modules": [
     {
       "source": "puppetlabs/deployment/ec",
@@ -140,8 +147,8 @@ var moduleVersionsMockResponse = `{
   ]
 }`
 
-// moduleDataMockResponse represents response from https://registry.terraform.io/v1/modules/puppetlabs/deployment/ec/0.0.8
-var moduleDataMockResponse = `{
+// puppetModuleDataMockResponse represents response from https://registry.terraform.io/v1/modules/puppetlabs/deployment/ec/0.0.8
+var puppetModuleDataMockResponse = `{
   "id": "puppetlabs/deployment/ec/0.0.8",
   "owner": "mattkirby",
   "namespace": "puppetlabs",
@@ -270,3 +277,215 @@ var moduleDataMockResponse = `{
     "0.0.8"
   ]
 }`
+
+// labelNullModuleVersionsMockResponse represents response for
+// versions of module that suffers from "unreliable" input data, as described in
+// https://github.com/hashicorp/vscode-terraform/issues/1582
+// It is a shortened response from https://registry.terraform.io/v1/modules/cloudposse/label/null/versions
+var labelNullModuleVersionsMockResponse = `{
+  "modules": [
+    {
+      "source": "cloudposse/label/null",
+      "versions": [
+        {
+          "version": "0.25.0",
+          "root": {
+            "providers": [],
+            "dependencies": []
+          },
+          "submodules": []
+        },
+        {
+          "version": "0.26.0",
+          "root": {
+            "providers": [],
+            "dependencies": []
+          },
+          "submodules": []
+        }
+      ]
+    }
+  ]
+}`
+
+// labelNullModuleDataOldMockResponse represents response for
+// a module that suffers from "unreliable" input data, as described in
+// https://github.com/hashicorp/vscode-terraform/issues/1582
+// It is a shortened response from https://registry.terraform.io/v1/modules/cloudposse/label/null/0.25.0
+var labelNullModuleDataOldMockResponse = `{
+  "id": "cloudposse/label/null/0.25.0",
+  "owner": "osterman",
+  "namespace": "cloudposse",
+  "name": "label",
+  "version": "0.25.0",
+  "provider": "null",
+  "provider_logo_url": "/images/providers/generic.svg?2",
+  "description": "Terraform Module to define a consistent naming convention by (namespace, stage, name, [attributes])",
+  "source": "https://github.com/cloudposse/terraform-null-label",
+  "tag": "0.25.0",
+  "published_at": "2021-08-25T17:47:04.039843Z",
+  "downloads": 52863192,
+  "verified": false,
+  "root": {
+    "path": "",
+    "name": "label",
+    "empty": false,
+    "inputs": [
+      {
+        "name": "environment",
+        "type": "string",
+        "default": "",
+        "required": true
+      },
+      {
+        "name": "label_order",
+        "type": "list(string)",
+        "default": "",
+        "required": true
+      },
+      {
+        "name": "descriptor_formats",
+        "type": "any",
+        "default": "{}",
+        "required": false
+      }
+    ],
+    "outputs": [
+      {
+        "name": "id"
+      }
+    ],
+    "dependencies": [],
+    "provider_dependencies": [],
+    "resources": []
+  },
+  "submodules": [],
+  "examples": [],
+  "providers": [
+    "null",
+    "terraform"
+  ],
+  "versions": [
+    "0.25.0",
+    "0.26.0"
+  ]
+}`
+
+// labelNullModuleDataOldMockResponse represents response for
+// a module that does NOT suffer from "unreliable" input data,
+// as described in https://github.com/hashicorp/vscode-terraform/issues/1582
+// This is for comparison with the unreliable input data.
+var labelNullModuleDataNewMockResponse = `{
+  "id": "cloudposse/label/null/0.26.0",
+  "owner": "osterman",
+  "namespace": "cloudposse",
+  "name": "label",
+  "version": "0.26.0",
+  "provider": "null",
+  "provider_logo_url": "/images/providers/generic.svg?2",
+  "description": "Terraform Module to define a consistent naming convention by (namespace, stage, name, [attributes])",
+  "source": "https://github.com/cloudposse/terraform-null-label",
+  "tag": "0.26.0",
+  "published_at": "2023-10-11T10:47:04.039843Z",
+  "downloads": 10000,
+  "verified": false,
+  "root": {
+    "path": "",
+    "name": "label",
+    "empty": false,
+    "inputs": [
+      {
+        "name": "environment",
+        "type": "string",
+        "default": "",
+        "required": true
+      },
+      {
+        "name": "label_order",
+        "type": "list(string)",
+        "default": "null",
+        "required": false
+      },
+      {
+        "name": "descriptor_formats",
+        "type": "any",
+        "default": "{}",
+        "required": false
+      }
+    ],
+    "outputs": [
+      {
+        "name": "id"
+      }
+    ],
+    "dependencies": [],
+    "provider_dependencies": [],
+    "resources": []
+  },
+  "submodules": [],
+  "examples": [],
+  "providers": [
+    "null",
+    "terraform"
+  ],
+  "versions": [
+    "0.25.0",
+    "0.26.0"
+  ]
+}`
+
+var labelNullExpectedOldModuleData = &tfregistry.ModuleData{
+	Version: version.Must(version.NewVersion("0.25.0")),
+	Inputs: []tfregistry.Input{
+		{
+			Name:        "environment",
+			Type:        cty.String,
+			Description: lang.Markdown(""),
+		},
+		{
+			Name:        "label_order",
+			Type:        cty.DynamicPseudoType,
+			Description: lang.Markdown(""),
+		},
+		{
+			Name:        "descriptor_formats",
+			Type:        cty.DynamicPseudoType,
+			Description: lang.Markdown(""),
+		},
+	},
+	Outputs: []tfregistry.Output{
+		{
+			Name:        "id",
+			Description: lang.Markdown(""),
+		},
+	},
+}
+
+var labelNullExpectedNewModuleData = &tfregistry.ModuleData{
+	Version: version.Must(version.NewVersion("0.26.0")),
+	Inputs: []tfregistry.Input{
+		{
+			Name:        "environment",
+			Type:        cty.String,
+			Description: lang.Markdown(""),
+			Required:    true,
+		},
+		{
+			Name:        "label_order",
+			Type:        cty.DynamicPseudoType,
+			Description: lang.Markdown(""),
+			Default:     cty.NullVal(cty.DynamicPseudoType),
+		},
+		{
+			Name:        "descriptor_formats",
+			Type:        cty.DynamicPseudoType,
+			Description: lang.Markdown(""),
+		},
+	},
+	Outputs: []tfregistry.Output{
+		{
+			Name:        "id",
+			Description: lang.Markdown(""),
+		},
+	},
+}