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

Allow module output when configuring firewall #10668

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

jackwhelpton
Copy link

Floated as a possible fix for #10494, but very much a work in progress.

@jackwhelpton jackwhelpton changed the title [WIP] Allow module output when configuring firewall Allow module output when configuring firewall Dec 2, 2021
@rileykarson rileykarson requested review from slevenick and removed request for rileykarson December 2, 2021 21:01
@rileykarson
Copy link
Collaborator

@slevenick, if you don't mind reviewing this! One other thing to look into is NewValueKnown (https://github.com/hashicorp/terraform-plugin-sdk/blob/main/helper/schema/resource_diff.go#L416-L423), which can help distinguish between values that are known / unknown. Unknown values will get returned as their zero value in a CustomizeDiff, and I forget what GetOkExists will report.

@jackwhelpton
Copy link
Author

Any updates on this?

Copy link
Collaborator

@slevenick slevenick left a comment

Choose a reason for hiding this comment

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

So, this seems to not quite work. If a user specifies source_ranges during creation and then removes it from their config, Terraform does not throw an error because it finds that source_ranges exists (due to the computed value being in state)

But, I think this is a better behavior than what currently exists, and I'm not really sure that we can handle this in a better way

@jackwhelpton
Copy link
Author

jackwhelpton commented Dec 8, 2021

Ah, I see the problem. I tested the scenario where no source_ranges exists to begin with, but not what happens if it's initially there and then removed. I think I agree that this proposal may be better than the current behavior, as at the moment perfectly valid setups are failing, something that can be seen in the examples for other modules.

I am curious if we can do better, though... I'm new to the provider codebase so I'll take your steer on that.

Also wondering if we should close out this PR in favor of GoogleCloudPlatform/magic-modules#5526.

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

Successfully merging this pull request may close these issues.

None yet

3 participants