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/kubernetes_persistent_volume: Add support for storage_class_name #111

Merged
merged 3 commits into from
Feb 26, 2018
Merged

resource/kubernetes_persistent_volume: Add support for storage_class_name #111

merged 3 commits into from
Feb 26, 2018

Conversation

sergei-ivanov
Copy link
Contributor

@sergei-ivanov sergei-ivanov commented Feb 5, 2018

It is basically a partial rebase of #72 plus a few additional fixes to make it actually work.

I have tested it against minikube by creating PVs with the source host_path, which is the simplest setup I could produce locally without hitting the cloud.

The testing revealed that creating a PV without a storage class and then amending it to the one with a storage class did not work because Kubernetes sent back a JSON without /spec/storageClassName path in it. That resulted in:

Error: Error applying plan:
1 error(s) occurred:
* kubernetes_persistent_volume.example: 1 error(s) occurred:
* kubernetes_persistent_volume.example: jsonpatch replace operation does not apply: doc is missing key: /spec/storageClassName

I had to tweak patchPersistentVolumeSpec a bit to produce either add or replace operation, which worked a treat in the end: I could add, change or remove storage class in the terraform manifest, and kubernetes applied the changes to the persistent volume.
I am not very experienced in Go, and there may be a more elegant way of doing what I've done,
so please feel free to correct me here.

Closes #72

@sergei-ivanov
Copy link
Contributor Author

This should partially address #74.
To fully address it, I think I'll need to roll another PR that will allow specifying empty storage class on PVC (there is a difference in binding behaviour between specifying an empty storage class and specifying no storage class at all).

@sergei-ivanov
Copy link
Contributor Author

I had a go at trying to allow empty or null storage class name for persistent volume claims, but that was thwarted by the fact that storage_class_name is marked as a computed attribute in the Terraform schema for PVCs. Which means, there is no way to change from a specific storage class to default or empty storage class. I guess it has to stay that way, because the only other option is to query for a storage class that has storageclass.kubernetes.io/is-default-class: true and use that to suppress diffs.

@fromz
Copy link

fromz commented Feb 26, 2018

This patch worked perfectly for me, thanks!

@radeksimko radeksimko added the bug label Feb 26, 2018
@radeksimko radeksimko changed the title Support storage_class_name in kubernetes_persistent_volume resource/kubernetes_persistent_volume: Add support for storage_class_name Feb 26, 2018
@radeksimko radeksimko self-assigned this Feb 26, 2018
Copy link
Member

@radeksimko radeksimko left a comment

Choose a reason for hiding this comment

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

Thanks for the PR @sergei-ivanov

The code looks overall good, I just added missing test which exercises the new field and also changed a few existing tests which were previously failing (and your PR allowed us to fix them 🎉 ), so we can pull this in. Feel free to ask any questions about my two commits.

@@ -766,13 +772,27 @@ func patchPersistentVolumeSpec(pathPrefix, prefix string, d *schema.ResourceData
Value: expandPersistentVolumeAccessModes(v.List()),
})
}
if d.HasChange(prefix + "access_modes") {
if d.HasChange(prefix + "persistent_volume_reclaim_policy") {
Copy link
Member

Choose a reason for hiding this comment

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

😱 good catch!

@radeksimko radeksimko merged commit aa7fa07 into hashicorp:master Feb 26, 2018
@sergei-ivanov
Copy link
Contributor Author

@radeksimko , thank you for looking after the tests. I did not have a spare GCP project to run them against, so I did not dare touch them. I ran a lot of manual checks against my local minikube though. Everything looks great, thank you for accepting the PR, we are looking forward to it being rolled into the next release of the provider!

@sergei-ivanov sergei-ivanov deleted the pv_storage_class_name branch February 27, 2018 02:27
@fromz
Copy link

fromz commented Feb 27, 2018

Thanks guys!

@coderfi
Copy link

coderfi commented Feb 27, 2018

Thanks for picking up the work, @sergei!

@ghost ghost locked and limited conversation to collaborators Apr 21, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants