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

Set allowed and cache methods as non nullable #324

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

travis-reed
Copy link

@travis-reed travis-reed commented Sep 25, 2024

what

Set allowed_methods and cached_methods as non nullable

Setting nullable to false ensures that the variable value will never be null within the module. If nullable is false and the variable has a default value, then Terraform uses the default when a module input argument is null.

why

I want to be able to sometimes call this module with explicit allowed_methods and cached_methods and sometimes just use the module defaults.

As it stands, I cannot do that without making my default value match your default value. It would be better for the module to use its defaults when I pass in null

Right now I am hitting

Error: Missing required argument

  with module.fanx.module.sdp_assets.module.static_cdn.aws_cloudfront_distribution.default[0],
  on /tmp/terraform-data-dir/modules/fanx.sdp_assets.static_cdn/main.tf line 522, in resource "aws_cloudfront_distribution" "default":
 522:     allowed_methods            = var.allowed_methods

The argument "default_cache_behavior.0.allowed_methods" is required, but no
definition was found.

Which I can work around by setting a default on my side, but it isn't ideal behavior

references

Additional Notes

I wouldn't consider this a breaking change. Today, the behavior if you pass in null as the argument to the module you will get a failure as shown above. This makes passing in null possible without negatively impacting existing users.

Setting nullable to false ensures that the variable value will never be null within the module. If nullable is false and the variable has a default value, then Terraform uses the default when a module input argument is null.
@travis-reed travis-reed requested review from a team as code owners September 25, 2024 21:55
@mergify mergify bot added the triage Needs triage label Sep 25, 2024
@Gowiem
Copy link
Member

Gowiem commented Sep 26, 2024

Huh this is interesting @travis-reed -- the first use-case of 'nullable' I've seen. Thanks for providing the explanation!

What version of TF was this introduced? We may need to update our pinning of TF for this module to support this usage depending on when it was added to the language.

@Gowiem
Copy link
Member

Gowiem commented Sep 26, 2024

/terratest

@travis-reed
Copy link
Author

travis-reed commented Sep 26, 2024

Huh this is interesting @travis-reed -- the first use-case of 'nullable' I've seen. Thanks for providing the explanation!

What version of TF was this introduced? We may need to update our pinning of TF for this module to support this usage depending on when it was added to the language.

Thanks for taking a look @Gowiem,

This feature was introduced in terraform 1.1

It looks like this module is requiring >= terraform 1.3 so I would expect this to work without upgrading the required version on the module

Screenshot 2024-09-25 at 11 27 00 PM

@travis-reed
Copy link
Author

I see that the bats action failed, but it seems like that may be failing on older merged prs too.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
triage Needs triage
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants