-
Notifications
You must be signed in to change notification settings - Fork 78
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
Allow parsing module from any filesystem #49
Conversation
ae57c80
to
06a2c6c
Compare
Severity: hcl.DiagError, | ||
Summary: "Failed to read file", | ||
Detail: fmt.Sprintf("The configuration file %q could not be read.", filename), | ||
}) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This error matches the one from ParseHCLFile
: https://github.com/hashicorp/hcl/blob/350d663f3c09e5a6d72ca11cb3250c1ce395bc8f/hclparse/parser.go#L70-L79
There is a small drift from ParseJSONFile
in that json.ParseFile
first opens the file and reports early diagnostic from there. This difference seems insignificant to me, but I'm open to retrofitting the extra Open
call with that extra dedicated diagnostic here if deemed necessary.
} else { | ||
file, fileDiags = parser.ParseHCLFile(filename) | ||
file, fileDiags = parser.ParseHCL(b, filename) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These original Parse*File
methods also provided "cache" of AST for each file which this change effectively removes.
It's just filename-based cache and so in order to get a hit we'd need two duplicate files in a directory, which is unlikely to ever happen anyway, so this change should in that sense be harmless and shouldn't affect performance in any way.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for including the clarifying comments in the diff! LGTM
* deps: Migrate from github.com/hashicorp/hcl2 to github.com/hashicorp/hcl/v2 (hashicorp#34) Reference: https://github.com/hashicorp/hcl2#experimental-hcl2 * contributing: terraform-config-inspect is feature complete (hashicorp#35) * terraform.required_providers provider source (hashicorp#37) * tfconfig: add parser for new (optional) required_providers syntax This is a breaking change: module.ProviderRequirements is now a map[string]ProviderRequirement instead of map[string][]string, and the json output has changed accordingly. The following syntaxes are now supported: terraform { required_providers { // new syntax "cloud" = { source = "hashicorp/cloud" version = "1.0.0" } // original syntax is also supported "cloud" = "1.0.0" } } * Fix Markdown rendering * Cleanup Readme formatting * Standardize directory for test data Use standard name for fixtures dir per Go conventions https://golang.org/cmd/go/#hdr-Test_packages * Migrate to Circle * fix: disambiguate variable defaults with "empty" values from undefined (hashicorp#24) - Variable struct and json representation now have an explicit `required` field to disambiguate between a variable with a default value which may either be `null` or the variable type's zero value * Merge source across required_providers blocks 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 constraint "1.0.0", but no "source". This commit merges the source attribute from the second block into this requirement. * Multiple provider source attributes is an error Previously we were diagnosing multiple provider different provider source attribute values as a warning, but this is really a configuration error. This commit updates the diagnostic to be an error, and adds a forced failure in the legacy parser when a `required_providers` block is encountered to ensure that the error propagates. * Allow parsing module from any filesystem (hashicorp#49) * Allow parsing from any filesystem * avoid caching parsed files * Expose lower-level hcl.File load function (hashicorp#53) * Support the sensitive attribute on outputs (hashicorp#55) (hashicorp#56) Add a "sensitive" attribute to outputs. If sensitive is not specified or is false it will not be present in the Output object as per omitempty behavior. * tfconfig: decode provider aliases (hashicorp#54) * allow parsing of required_providers containing ref The syntax for configuration_aliases contains bare references to match their use in other parts of the configuration. These however cannot be decoded directly without an EvalContext, as they represent variables. Refactor decodeRequiredProvidersBlock to use the lower level ExprMap function. * Expose configuration_aliases from required_providers (hashicorp#60) * README: The latest releases of this library support the Terraform v0.15 language * README: This library is compatible with the Terraform 1.0 language * README: Updated note about compatibility We now have 1.0 compatibility promises, so we can be more specific in what this library can do. * Parse the sensitive key for input variables * Remove Sensitive property for legacy modules * Update CircleCI config to fix build failures * Give up on 1.11.13, replace with 1.17.3 * update circle config and docker mirror * Fixup after merge Co-authored-by: Brian Flad <bflad417@gmail.com> Co-authored-by: Kristin Laemmert <mildwonkey@users.noreply.github.com> Co-authored-by: Radek Simko <radek.simko@gmail.com> Co-authored-by: Michele <mdeggies@gmail.com> Co-authored-by: Jonathan Stewmon <jstewmon@gmail.com> Co-authored-by: Alisdair McDiarmid <alisdair@users.noreply.github.com> Co-authored-by: Andy Caruso <63117216+andy-caruso@users.noreply.github.com> Co-authored-by: James Bardin <j.bardin@gmail.com> Co-authored-by: Martin Atkins <mart@degeneration.co.uk> Co-authored-by: Kyle Carberry <kyle@carberry.com>
Summary
This PR introduces two new interfaces to aid with accessing configuration in any "abstract" filesystem, such as in-memory FS which is used by the language server. In LSP the server should not read files from disk if there is copy in memory (as sent by the client).
I created interfaces pretty much identical to the ones recently published in the Go proposal for
io/fs
. We don't actually needOpen
norFile
, but I believe that adding later anything to an interface is going to be much harder than removing anything. Also assuming that the proposal goes ahead and is implemented like this, it should be fairly easy to just swap the argument types for the stdlib ones and add type aliases.Two new functions which use this new interface were also introduced:
LoadModuleFromFilesystem(fs FS, dir string) (*Module, Diagnostics)
LoadModule(dir string) (*Module, Diagnostics)
IsModuleDirOnFilesystem(fs FS, dir string) bool
IsModuleDir(dir string) bool
Backwards Compatibility for Existing Consumers
The existing interfaces should work the same as before.