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

Do not send 0 subnet value unless explicitly set #496

Merged
merged 3 commits into from
Nov 4, 2021

Conversation

smaeda-ks
Copy link
Contributor

@smaeda-ks smaeda-ks commented Nov 3, 2021

After we changed the subnet type to int:
fastly/go-fastly#288
and updated go-fastly version:
https://github.com/fastly/terraform-provider-fastly/blob/main/CHANGELOG.md#0350-september-15-2021

We started sending 0 subnet value even if the value is not set. This is because convertSubnetToInt returns the default value (0) when passing an empty string (""). Fixing this to only send 0 value when the subnet attribute is explicitly set by the user.

Also, flattenAclEntries function needs to know if the returned subnet value is null or any int value in order to track them properly in the state file. To do so, we need to make a change in the upstream go-fastly package:
fastly/go-fastly#316

Otherwise, we always see 0 subnet value regardless of the actual returned value (i.e., null vs 0 int value) and save it to the state file, which causes a diff loop.


subnet := convertSubnetToInt(v["subnet"].(string))
// only set zero subnet if the attribute is explicitly set
if v["subnet"].(string) == "0" || subnet != 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it might be cleaner to move this logic inside convertSubnetToInt? Then convertSubnetToInt could return *int

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think I'm okay with the current

@bengesoff
Copy link
Contributor

Looks like some of the implementation needs fixing but the principle seems reasonable!

@Integralist Integralist self-requested a review November 4, 2021 11:09
@Integralist Integralist added the bug label Nov 4, 2021
@smaeda-ks smaeda-ks marked this pull request as ready for review November 4, 2021 17:47
@smaeda-ks smaeda-ks self-assigned this Nov 4, 2021
@smaeda-ks
Copy link
Contributor Author

@bengesoff @Integralist
I've added one last commit to bump the go-fastly version to v5.1.2, which is necessary for this PR.

Copy link
Collaborator

@Integralist Integralist left a comment

Choose a reason for hiding this comment

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

LGTM

@smaeda-ks smaeda-ks merged commit bb6b0c6 into fastly:main Nov 4, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants