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

Specify all variables as non-nullable #221

Merged
merged 4 commits into from
Nov 8, 2024

Conversation

jsf9k
Copy link
Member

@jsf9k jsf9k commented Oct 9, 2024

🗣 Description

This pull request specifies all variables as non-nullable.

💭 Motivation and context

This means that we need not worry about whether variables are explicitly specified as null. Since this is not something we were expecting, it makes sense to avoid the situation.

See also the Terraform documentation.

🧪 Testing

All automated tests pass.

✅ Pre-approval checklist

  • This PR has an informative and human-readable title.
  • Changes are limited to a single goal - eschew scope creep!
  • All relevant type-of-change labels have been added.
  • I have read the CONTRIBUTING document.
  • These code changes follow cisagov code standards.
  • All new and existing tests pass.

This means that we need not worry about whether variables are
explicitly specified as null.  Since this is not something we were
expecting, it makes sense to avoid the situation.

See also:
https://developer.hashicorp.com/terraform/language/values/variables#disallowing-null-input-values
@jsf9k jsf9k added improvement This issue or pull request will add or improve functionality, maintainability, or ease of use terraform Pull requests that update Terraform code hacktoberfest-accepted Pull request that should count toward Hacktoberfest participation kraken 🐙 This pull request is ready to merge during the next Lineage Kraken release labels Oct 9, 2024
@jsf9k jsf9k marked this pull request as ready for review October 9, 2024 16:57
@jsf9k jsf9k requested a review from a team October 9, 2024 16:57
@dav3r
Copy link
Member

dav3r commented Oct 9, 2024

The nullable argument is only available in Terraform v1.1.0 and later, but our skeleton is defined as required_version = "~> 1.0". If we want this feature, we need to update our required_version. Anyone have any concerns with doing that?

@jsf9k
Copy link
Member Author

jsf9k commented Oct 9, 2024

Version 1.1 of Terraform is the first version to support the nullable
key in variable definitions.

Also update the comment associated with this line to something a bit
more truthful, since we are using 1.5.7 in
cisagov/setup-env-github-action.

Co-authored-by: David Redmin <[email protected]>
@jsf9k jsf9k force-pushed the improvement/specify-variables-as-non-nullable branch from 43ef99c to c551420 Compare October 9, 2024 18:29
@jsf9k
Copy link
Member Author

jsf9k commented Oct 9, 2024

The nullable argument is only available in Terraform v1.1.0 and later, but our skeleton is defined as required_version = "~> 1.0". If we want this feature, we need to update our required_version. Anyone have any concerns with doing that?

See commit c551420.

@dv4harr10
Copy link
Contributor

Hi Shane @jsf9k, the Readme file still shows the reference to terraform ~> 1.0

@jsf9k
Copy link
Member Author

jsf9k commented Oct 9, 2024

Hi Shane @jsf9k, the Readme file still shows the reference to terraform ~> 1.0

See commit e57dd10, which I just pushed up.

@dv4harr10
Copy link
Contributor

Hi Shane @jsf9k, under directory examples/basic_usage: same for Readme file and version.tf there reference to terraform ~> 1.0

@jsf9k jsf9k force-pushed the improvement/specify-variables-as-non-nullable branch from f9bf064 to 139eefd Compare October 9, 2024 19:38
@jsf9k
Copy link
Member Author

jsf9k commented Oct 9, 2024

Hi Shane @jsf9k, under directory examples/basic_usage: same for Readme file and version.tf there reference to terraform ~> 1.0

Please see commit 193083a.

@jsf9k jsf9k force-pushed the improvement/specify-variables-as-non-nullable branch from 139eefd to 193083a Compare October 9, 2024 19:40
Copy link
Member

@mcdonnnj mcdonnnj left a comment

Choose a reason for hiding this comment

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

Seems reasonable enough.

Copy link
Member

@dav3r dav3r left a comment

Choose a reason for hiding this comment

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

LGTM

@mcdonnnj mcdonnnj added this pull request to the merge queue Nov 8, 2024
Merged via the queue into develop with commit d0a08ae Nov 8, 2024
4 checks passed
@mcdonnnj mcdonnnj deleted the improvement/specify-variables-as-non-nullable branch November 8, 2024 09:44
mcdonnnj added a commit that referenced this pull request Nov 13, 2024
In #221 we made all of the variables in the example module non-nullable
but did not do the same to the `basic_usage` example. Since it uses
variables beyond the requirements of the example module it makes sense
to also specify the example's variables as non-nullable.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hacktoberfest-accepted Pull request that should count toward Hacktoberfest participation improvement This issue or pull request will add or improve functionality, maintainability, or ease of use kraken 🐙 This pull request is ready to merge during the next Lineage Kraken release terraform Pull requests that update Terraform code
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

4 participants