Merge source across required_providers blocks

[alisdair]

If multiple terraform.required_providers blocks are in the config, we already merge the version constraints for each provider. We should also merge the source attribute, and warn if there are duplicates.

Consider the following configuration:

terraform {
  required_providers {
    foo = {
      version = "1.0.0"
    }
  }
}

terraform {
  required_providers {
    foo = {
      source = "abc/foo"
    }
  }
}

Before this commit, this would result in a provider requirement for "foo" with version "1.0.0", but no source. This commit merges the source attribute from the second block into this requirement.

[mildwonkey]

Looks great! Only one concern: I think this should be an error, not a warning. What do you think?

[alisdair]

I agree, but I can't convince it to work. If I use a DiagError severity, the output for this test skips the required providers section altogether:

--- FAIL: TestRenderMarkdown (0.01s)
    --- FAIL: TestRenderMarkdown/provider-source-invalid (0.00s)
        markdown_test.go:48:
            # Module `testdata/provider-source-invalid`

             !=
            # Module `testdata/provider-source-invalid`

            Provider Requirements:
            * **bat (`abc/bat`):** `1.0.0`
            * **foo (`abc/foo`):** `~> 2.0.0, ~> 2.1.0`

            ## Problems

            ## Warning: Multiple provider source attriubtes

            (at `testdata/provider-source-invalid/provider-source-invalid.tf` line 15)

            Found multiple source attributes for provider bat: "abc/bat", "baz/bat"

I believe this is because of this fallback code if we generate an error with our loader:

terraform-config-inspect/tfconfig/load.go

Lines 24 to 31 in 36d6f40

	if diags.HasErrors() {
	// Try using the legacy HCL parser and see if we fare better.
	legacyModule, legacyDiags := loadModuleLegacyHCL(dir)
	if !legacyDiags.HasErrors() {
	legacyModule.init(legacyDiags)
	return legacyModule, legacyDiags
	}
	}

Since the legacy parser doesn't generate an error, we end up skipping the required providers block altogether 😕

I'm not sure how to address that. Do you have any ideas?

[mildwonkey]

I'm not sure I follow - the test output you've posted looks correct to me; could you write the test that expects an error and specifically check for that error?

Found multiple source attributes for provider bat: "abc/bat", "baz/bat"

[alisdair]

Sorry, I wasn't clear (and neither is the test output). This is from me running the new test on this branch (files here), which fails when I change the severity to error. The failure mode is an empty markdown file, rather than "warning" != "error" as I'd hoped.

That is, the output is:

# Module `testdata/provider-source-invalid`

instead of:

# Module `testdata/provider-source-invalid`

Provider Requirements:
* **bat (`abc/bat`):** `1.0.0`
* **foo (`abc/foo`):** `~> 2.0.0, ~> 2.1.0`

## Problems

## Error: Multiple provider source attriubtes

(at `testdata/provider-source-invalid/provider-source-invalid.tf` line 15)

Found multiple source attributes for provider bat: "abc/bat", "baz/bat"

[apparentlymart]

I think what you're seeing here is an unfortunate interaction between the two different parsing modes in this library...

terraform-config-inspect first parses with the HCL 2-based parser. If it gets an error, it then tries to parse with the HCL 1-based parser. If the latter succeeds then its result is returned. If the latter fails then the whole module is considered to be in error and we return the errors from the HCL 2-based parser because we expect them to be better.

The behavior you're reporting above looks to me like the HCL 1-based parser is succeeding, presumably because it doesn't know about the required_providers block (that was a Terraform 0.12 feature) and is just ignoring it.

To make this fail in the way you want, I think it might be necessary to add an extra check to the HCL 1-based parser to force it to always fail if a required_providers block is present, thus ensuring it can never "win" and suppress the errors returned by the HCL 2-based parser. That's gross, but I think it will work, if I'm right about the root cause.

[mildwonkey]

The test cases in TestRenderMarkdown (historically) are only testing valid modules, and other tests check for expected errors on config load.

In looking at your branch I'd say that this is correct: the markdown should fail to render (ie, there should be no valid *.out.json to even test against) since we expect an error in that situation.

[alisdair]

@apparentlymart Thanks! I added the extra check to the legacy parser in b19c2c0 and now we have an error instead of a warning 🎉

@mildwonkey I think this might be the right approach now…? We do still render a Markdown file, but it includes the error diagnostic, so hopefully that's the desired behaviour.

[mildwonkey]

I have concerns about returning a possibly-inaccurate result (in markdown) alongside the error. I would expect no output, or at least empty "required_providers" output, in this case.

I'm not sure how this tool is getting used, so I could be way off on my risk assessment, but my fear is seeing inaccurate data in the registry, or causing issue with other third-party tools that assume if they get a result, the config was valid, and therefor they do not check for diagnostic errors.

Having said this, it looks great, and if that's consistent with how terraform_config_inspect renders markdown in the face of other errors I won't make a fuss!

[alisdair]

I think it's consistent with existing behaviour to still render a Markdown file (and a JSON file). Here's an example of existing Markdown test output, and corresponding JSON which includes nodes for invalid objects.

But I'm also not sure how the tool is used, and if we need to change that behaviour then I'd be happy to do so.

[mildwonkey]

I don't know why I didn't think of this yesterday, but I generally ask @justincampbell to review PRs to this codebase since it's used in the registry. If he doesn't have any concerns about this change that's good enough for us!

[mildwonkey]

I approve, looks great and thank you.

We should get a second approval from someone who works on the public registry.

[justincampbell]

Thanks! I checked our ingress tests against this dependency and everything seems to pass. I don't foresee any issues with this change. Look good!

[justincampbell]

Also FWIW, I don't believe the Terraform Registry codebase uses any Markdown output from this library.

[alisdair]

After some more discussion about how this would be used in Terraform, we're not going ahead with this change.