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

Add module source address parsing #7

Merged
merged 4 commits into from
Apr 22, 2022
Merged

Add module source address parsing #7

merged 4 commits into from
Apr 22, 2022

Conversation

dbanck
Copy link
Member

@dbanck dbanck commented Apr 20, 2022

This PR adds the parsing of module source addresses for registry modules. It exposes one new function:

  • ParseRawModuleSourceRegistry

It's based on the internal/addrs/module_source.go code in terraform core with some modifications:

  • Removed anything related to ModuleSourceRemote (e.g. GitHub or Bitbucket)
  • Removed anything related to ModuleSourceLocal (e.g. ./example)
  • Removed the go-getter dependency

Example usage

moduleSourceRegistry, err := tfaddr.ParseRawModuleSourceRegistry(module.SourceAddr)
if err == nil {
	bodySchema.DocsLink = &schema.DocsLink{
		URL: fmt.Sprintf(
			`https://%s/modules/%s/%s`,
			moduleSourceRegistry.PackageAddr.Host.ForDisplay(),
			moduleSourceRegistry.PackageAddr.ForRegistryProtocol(),
			"latest",
		),
	}
}

Enables hashicorp/terraform-ls#877

@dbanck dbanck force-pushed the f-add-module-add branch 3 times, most recently from 74e07a4 to bec8935 Compare April 20, 2022 15:26
@dbanck dbanck added the enhancement New feature or request label Apr 20, 2022
@dbanck dbanck self-assigned this Apr 20, 2022
This enabled parsing of module source addresses for local and
registry modules.
@dbanck dbanck marked this pull request as ready for review April 21, 2022 08:53
@dbanck dbanck changed the title WIP: add module source address parsing Add module source address parsing Apr 21, 2022
@dbanck dbanck requested a review from radeksimko April 21, 2022 08:53
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

This LGTM aside from one question/aspect:

Is there a particular reason for including ParseRawModuleSource, ModuleSourceLocal and the ModuleSource interface? The detection of local modules doesn't seem all that complicated and I think we could get away with it living either in terraform-ls, or potentially under terraform-schema: https://github.com/hashicorp/terraform-ls/blob/94aa1d8d66f28441fbf8e3273eefcc5554b9b817/internal/terraform/datadir/module_types.go#L61-L68

I guess there may be a convenience argument for including it, but the risk is that this gives anyone the expectation that the function can parse any valid module source address and I'd really prefer to avoid that scope creep, if possible - esp. because that allows us to avoid go-getter.

Maybe we'll re-scope the library as hashicorp/terraform-address at some point, but I'd prefer not to do that unless it's really necessary.

@dbanck
Copy link
Member Author

dbanck commented Apr 22, 2022

Good call! I've removed the code related to local module parsing, and without the ModuleSource interface, usage of this library becomes much easier.

I updated the example and removed the type cast.

@dbanck dbanck requested a review from radeksimko April 22, 2022 08:48
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

One more question + docs fix 😅

module.go Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@dbanck dbanck requested a review from radeksimko April 22, 2022 09:17
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

:shipit: 🚀

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants