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.

Sign up for GitHub

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

Jump to bottom

fix: disambiguate variable defaults with "empty" values from undefined #24

Merged
merged 1 commit into from
May 18, 2020
Merged

fix: disambiguate variable defaults with "empty" values from undefined #24

merged 1 commit into from
May 18, 2020

Conversation

jstewmon
Copy link
Contributor

closes #23

@jstewmon

This comment has been minimized.

Sign in to view
@jstewmon

This comment has been minimized.

Sign in to view
@jstewmon

This comment has been minimized.

Sign in to view
@jstewmon
Copy link
Contributor Author

I figured out that just wrapping the Default in a struct was sufficient to disambiguate whether or not a default was defined and is trivially unwrapped with a custom MarshalJSON. 😄

@jstewmon jstewmon changed the title fix: variables with "empty" default values should not be marked as required fix: disambiguate variable defaults with "empty" values from undefined Jul 12, 2019
@jstewmon
Copy link
Contributor Author

@mildwonkey would you be so kind as to take a look at this and provide feedback? We're getting a lot of leverage out of this project, especially maintaining module docs, and would love to have the variable requirements accurately documented. 🙏

@moritzheiber
Copy link

@jstewmon Great work, it's been annoying to see this happening
@radeksimko Would love to get this merged if you have some time ❤️

@willychenchen
Copy link

Could we get this reviewed and merged in, please? Thank you.

@mildwonkey
Copy link
Contributor

Hi @jstewmon ! Thank you for submitting this PR.

I understand and agree with the desire to differentiate between unset and empty default values. However I'd like to suggest a different approach, which is more in line with how Terraform itself models that idea: add a boolean for whether the variable is required (i.e. true if there is no default value; false when there is a default, even if the default is "empty"). Does that sound reasonable to you?

@jstewmon
Copy link
Contributor Author

@mildwonkey , sure - I'll add a new commit, so we can review how that looks.

I didn't do that originally b/c I thought it would be less intuitive to remove omitempty from Default, so that you'd always have Default and have to check another field to determine if Default is significant.

But, sounds like there is more upside to doing it that way to align more closely with Terraform.

@jstewmon
Copy link
Contributor Author

@mildwonkey new commit to add required field is ready for you to review!

@jstewmon
Copy link
Contributor Author

jstewmon commented Dec 9, 2019

Hi @mildwonkey 👋 would love to get your feedback on the new approach when you have a few minutes. 🙏

@matsest matsest mentioned this pull request Jan 21, 2020
Closed
1 task
@khos2ow
Copy link

khos2ow commented Feb 3, 2020 •
edited
Loading

Is there any ETA on this? We have a feature request at terraform-docs/terraform-docs#176 which depends on this and I'd rather use the functionality right from terraform-config-insoect than try to re-implement it within terraform-docs. Also I can help on anything on this PR if nees be.

khos2ow added a commit to khos2ow/terraform-docs that referenced this pull request Mar 22, 2020
@khos2ow
Move github.com/hashicorp/terraform-config-inspect to internal/tfconfig
aad2264 
Internally we depend on terraform-config-inspect, but at the moment
they state that they consider the project is feature-complete and
they do not accept any enhancement pull requests, as such we've
decided to bring over the project as internal package here rather than
being a vendor dependency and apply the fix from @jstewmon from
hashicorp/terraform-config-inspect#24 directly
here.

Since the terraform-config-inspect is considered to be feature-complete
we don't expect to have any more changes on the package, and if there
was a change on upstream we're going to bring it down in the
corresponding package.
This was referenced Mar 22, 2020
Merged
Closed
khos2ow added a commit to khos2ow/terraform-docs that referenced this pull request Mar 24, 2020
@khos2ow
Move github.com/hashicorp/terraform-config-inspect to internal/tfconfig
e101cbb 
Internally we depend on terraform-config-inspect, but at the moment
they state that they consider the project is feature-complete and
they do not accept any enhancement pull requests, as such we've
decided to bring over the project as internal package here rather than
being a vendor dependency and apply the fix from @jstewmon from
hashicorp/terraform-config-inspect#24 directly
here.

Since the terraform-config-inspect is considered to be feature-complete
we don't expect to have any more changes on the package, and if there
was a change on upstream we're going to bring it down in the
corresponding package.
khos2ow added a commit to terraform-docs/terraform-docs that referenced this pull request Mar 30, 2020
@khos2ow
fix: Mark variables not required if default set to null (#221)
a3e0a56 
* Mark variables not required if default set to null

* Move github.com/hashicorp/terraform-config-inspect to internal/tfconfig

Internally we depend on terraform-config-inspect, but at the moment
they state that they consider the project is feature-complete and
they do not accept any enhancement pull requests, as such we've
decided to bring over the project as internal package here rather than
being a vendor dependency and apply the fix from @jstewmon from
hashicorp/terraform-config-inspect#24 directly
here.

Since the terraform-config-inspect is considered to be feature-complete
we don't expect to have any more changes on the package, and if there
was a change on upstream we're going to bring it down in the
corresponding package.

* Add notice to the new package

* add license of terraform-config-inspect code

* fix tests after merging master

* fix test after merge master

* show 'required' attribute in JSON, XML, YAML

* update docs
@paultyng paultyng mentioned this pull request May 18, 2020
Closed
@mildwonkey
Copy link
Contributor

Hello @jstewmon and sorry again for the long wait! Thank you for making the latest update. I'd like to get this merged.
Do you want to update this PR and resolve the conflicts? If you aren't interested in working on this PR anymore (it has been a long time, I know!) I can resolve the merge conflicts.
Cheers!

@jstewmon jstewmon requested a review from a team as a code owner May 18, 2020 17:34
@jstewmon
Copy link
Contributor Author

Hi @mildwonkey , I've rebased against master. Would you like for me to squash out the original commit?

@mildwonkey
Copy link
Contributor

@jstewmon only if you want to re-write your commit history yourself - I usually use squash and merge, so it isn't necessary.

@jstewmon
fix: disambiguate variable defaults with "empty" values from undefined
27428fa 
- 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
mildwonkey
mildwonkey approved these changes May 18, 2020
View reviewed changes
Copy link
Contributor

@mildwonkey mildwonkey left a comment

Choose a reason for hiding this comment

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

Thank you for working on this @jstewmon , and your patience!

@mildwonkey mildwonkey merged commit 3a72ca6 into hashicorp:master May 18, 2020
@mrwacky42 mrwacky42 mentioned this pull request Feb 15, 2022
Merged
mrwacky42 added a commit to HeadspaceMeditation/terraform-config-inspect that referenced this pull request Feb 15, 2022
@mrwacky42 @bflad
@mildwonkey @radeksimko @mdeggies @jstewmon @alisdair @andy-caruso @jbardin @apparentlymart @kylecarbs
OPS-8302 Merge upstream to support TF 1.x syntax (#11)
7dbd629 
* 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mildwonkey mildwonkey mildwonkey approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

"empty" variable default values result in variable being marked as required
5 participants
@jstewmon @moritzheiber @willychenchen @mildwonkey @khos2ow