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

resource_volume : set topology_request on read #342

Merged
merged 4 commits into from
Jul 11, 2023

Conversation

the-nando
Copy link
Contributor

@the-nando the-nando commented Jul 8, 2023

Fixes: #338

The MR covers only required topology_request as that's what's implemented in the schema for this resource. Should it be extended to account also for preferred? According the the Volume Specification they are both supported.

@the-nando the-nando force-pushed the hotfix/volume_topology_requests branch 2 times, most recently from 6865a3f to e3f00bd Compare July 8, 2023 09:34
@lgfa29
Copy link
Contributor

lgfa29 commented Jul 10, 2023

Thanks for the PR @the-nando!

nomad_volume and nomad_external_volume are, due to historical reasons, two weird resources 😅

As noted in the docs and in the comment in the code (though the URL is broken, so we need to fix that), preferred is only supported on volume creation, which nomad_volume does not do, only nomad_external_volume creates volumes.

But the Read method is shared between them, so I think we do need to handle preferred so nomad_external_volume is fully fixed (and maybe add ForceNew there as well). Hopefully Terraform won't complain about the missing field in the schema, though I do think it might 😬

If it does complain, then I think we may need to add it to the schema but as computed so users don't set it.

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 11, 2023

After review this PR I realized that this may be a good time to clean-up the mess around the volume resources since we're planning a major release with some breaking changes, so I opened #344 to do that.

I could apply these changes to that PR (plus in the new resources), or you can rebase and work off that branch.

Let me know what would work best for you.

@the-nando
Copy link
Contributor Author

Hey @lgfa29, thanks for the clarification regarding the setting of preferred in nomad_volumes vs nomad_external_volume :)

Feel free to bundle up the changes in this PR with the ones in #344 and close this if that's easier for you. I'll wait then for the next major.

Thanks!

@lgfa29
Copy link
Contributor

lgfa29 commented Jul 11, 2023

Looking at it again, #344 was already big and convoluted enough that adding more changes to it would make it even more confusing 😅

So I think I will rebase your branch and keep this PR up for this specific bug fix.

@lgfa29 lgfa29 force-pushed the hotfix/volume_topology_requests branch from e3f00bd to 24b8b4b Compare July 11, 2023 20:32
Copy link
Contributor

@lgfa29 lgfa29 left a comment

Choose a reason for hiding this comment

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

I've updated the topology request flattening function to also read preferred and Terraform didn't seem to complain about it, so I didn't have to update the schema. I also noticed that ForceNew didn't work unless it was set in all levels of the schema, so I addedthat as well.

Lastly I rebased your branch to apply the fix to the new nomad_csi_volume and nomad_csi_volume_request resources, so i think we're all good here.

Free to reach out if you notice a problem with these changes.

Thanks again for the PR!

@lgfa29 lgfa29 merged commit 8ccd38e into hashicorp:main Jul 11, 2023
1 check passed
@the-nando the-nando deleted the hotfix/volume_topology_requests branch July 12, 2023 05:20
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.

resource_volume doesn't detect topology_request drift
2 participants