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

Preload providers #302

Merged
merged 6 commits into from
Nov 18, 2020
Merged

Preload providers #302

merged 6 commits into from
Nov 18, 2020

Conversation

appilon
Copy link
Contributor

@appilon appilon commented Nov 17, 2020

This PR creates a generate directive to

  • query the registery for the list of official providers
  • generate a config file with a required_providers block for all those providers (no version constraint, so TF will assume latest)
  • use terraform-exec to initialize/download all the providers
  • use terraform-exec to extract the provider schemas
  • use vfsgen to embed the json into a go file
  • modifies the root module package to use the preloaded provider schemas until the rootmodule schemas are loaded

May depend on #301

TODO

vfsgen uses a common pattern of alike tools in working with the http.FileSystem interface. It would be nice to find an alternative that can follow the new embed proposal, which builds off the new filesystem proposal (something we have begun to adopt). Perhaps we could fork vfsgen and reimplement it to work with the proposed types.

Closes #292

@appilon appilon requested a review from a team November 17, 2020 04:47
@radeksimko
Copy link
Member

vfsgen uses a common pattern of alike tools in working with the http.FileSystem interface. It would be nice to find an alternative that can follow the new embed proposal, which builds off the new filesystem proposal (something we have begun to adopt). Perhaps we could fork vfsgen and reimplement it to work with the proposed types.

I would hope/expect that if vfsgen is still an actively maintained library, they'll have interest in making it compatible with the native interfaces. Either way though I wouldn't worry about this too much until the proposal is actually implemented and available in a stable Go version.

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.

I left you some comments inline, the most important one being about the panic.

From practical stand point I would probably only stick to the official providers for now as the generated file already has 3.5M. I think it's a reasonable cost to pay for better UX 👍 , but we have to keep in mind that maintaining bigger files in the repo can negatively affect the longer-term developer experience (nobody enjoys cloning large repositories) and potentially also end-user UX (nobody enjoys downloading large binaries).

.gitignore Outdated Show resolved Hide resolved
.gitignore Outdated Show resolved Hide resolved
main.go Outdated Show resolved Hide resolved
internal/terraform/rootmodule/root_module.go Outdated Show resolved Hide resolved
internal/terraform/rootmodule/root_module.go Show resolved Hide resolved
schemas/gen/gen.go Outdated Show resolved Hide resolved
schemas/gen/gen.go Outdated Show resolved Hide resolved
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.

I apologise ahead for the conflicts, but I'd recommend merging #307 first and rebasing this PR.

internal/terraform/rootmodule/root_module.go Outdated Show resolved Hide resolved
internal/terraform/rootmodule/root_module.go Outdated Show resolved Hide resolved
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.

LGTM

🚢

internal/terraform/rootmodule/root_module.go Outdated Show resolved Hide resolved
Co-authored-by: Radek Simko <radek.simko@gmail.com>
@appilon appilon merged commit 33ae89e into master Nov 18, 2020
@appilon appilon deleted the preload-providers branch November 18, 2020 20:22
@ghost
Copy link

ghost commented Dec 18, 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 context necessary to investigate further.

@ghost ghost locked as resolved and limited conversation to collaborators Dec 18, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Preload core provider schemas
2 participants