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.

Sign up for GitHub

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

Jump to bottom

r/storage_class - add support for allowed_topologies + some validations #910

Closed
wants to merge 8 commits into from
Closed

r/storage_class - add support for allowed_topologies + some validations #910

wants to merge 8 commits into from

Conversation

DrFaust92
Copy link
Contributor

@DrFaust92 DrFaust92 commented Jul 13, 2020 •
edited
Loading

Description

Acceptance tests

  • Have you added an acceptance test for the functionality being added?
  • Have you run the acceptance tests on this branch? (If so, please include the test log in a gist)
--- PASS: TestAccKubernetesStorageClass_basic (23.41s)
--- PASS: TestAccKubernetesStorageClass_importBasic (6.34s)
--- PASS: TestAccKubernetesStorageClass_generatedName (6.52s)
--- PASS: TestAccKubernetesStorageClass_importGeneratedName (5.46s)
--- PASS: TestAccKubernetesStorageClass_allowedTopologies (4.11s)
--- PASS: TestAccKubernetesDataSourceStorageClass_basic (2.91s)

References

Closes #391, #891

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

@DrFaust92 DrFaust92 changed the title r/storage_class - add support for allowed_topologies + some validations [WIP]r/storage_class - add support for allowed_topologies + some validations Jul 13, 2020
dak1n1
dak1n1 reviewed Aug 21, 2020
View reviewed changes
Copy link
Contributor

@dak1n1 dak1n1 left a comment

Choose a reason for hiding this comment

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

This is coming along nicely! I left a couple comments as initial feedback.

kubernetes/resource_kubernetes_storage_class.go Outdated Show resolved Hide resolved
kubernetes/resource_kubernetes_storage_class_test.go
IDRefreshName: resourceName,
Providers: testAccProviders,
CheckDestroy: testAccCheckKubernetesStorageClassDestroy,
Steps: []resource.TestStep{
Copy link
Contributor

Choose a reason for hiding this comment

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

You might want to skip this check if the test isn't running on GKE, since the config contains some GKE specific elements. See this commit for an example https://github.com/dak1n1/terraform-provider-kubernetes/blob/c245d421be2e2070e8cb61a03a6cc4f026d5c07c/kubernetes/resource_kubernetes_persistent_volume_claim_test.go#L51-L63

Copy link
Contributor Author

Choose a reason for hiding this comment

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

its relevant for AWS as well.

Copy link
Member

Choose a reason for hiding this comment

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

Would the test run on AWS as is (with `storage_provisioner = "kubernetes.io/gce-pd")?
If not, you might want to insert the value of storage_provisioner as a function parameter to the config function and determine it based on runtime environment of the test.

Copy link
Contributor

Choose a reason for hiding this comment

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

I actually did a bunch of work in this file on Friday. I'm thinking I'll cherry-pick the changes into my branch here.

fe827fb

@DrFaust92 DrFaust92 changed the title [WIP]r/storage_class - add support for allowed_topologies + some validations r/storage_class - add support for allowed_topologies + some validations Sep 25, 2020
alexsomesan
alexsomesan reviewed Oct 9, 2020
View reviewed changes
Copy link
Member

@alexsomesan alexsomesan left a comment

Choose a reason for hiding this comment

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

Looks good. One last thing to clarify around the test configuration (see observation by @dak1n1 )

dak1n1 added a commit to dak1n1/terraform-provider-kubernetes that referenced this pull request Oct 12, 2020
@dak1n1
add allowed_topologies from PR 910
e7f863c 
hashicorp#910
@dak1n1 dak1n1 mentioned this pull request Oct 12, 2020
Merged
2 tasks
@dak1n1
Copy link
Contributor

dak1n1 commented Oct 12, 2020

I got the changes into my PR here fe827fb

The acceptance test is passing. Thanks for this feature!

@dak1n1 dak1n1 closed this Oct 12, 2020
dak1n1 added a commit to dak1n1/terraform-provider-kubernetes that referenced this pull request Oct 19, 2020
@dak1n1
Update to Terraform SDK v2
26f1cae 
* Update import paths and functions to use Terraform SDKv2.
* Add support for testing multiple versions of the provider.
* Add `allowed_topologies` to the storageclass resource,
  authored by Ilia Lazebnik in PR
  hashicorp#910
dak1n1 added a commit to dak1n1/terraform-provider-kubernetes that referenced this pull request Oct 23, 2020
@dak1n1
Update to Terraform SDK v2
b8e5aa1 
* Update import paths and functions to use Terraform SDKv2.
* Add support for testing multiple versions of the provider.
* Add `allowed_topologies` to the storageclass resource,
  authored by Ilia Lazebnik in PR
  hashicorp#910
* Fix acceptance tests and add upgrade tests.
dak1n1 added a commit to dak1n1/terraform-provider-kubernetes that referenced this pull request Oct 29, 2020
@dak1n1
Update to Terraform SDK v2
0d15562 
* Update import paths and functions to use Terraform SDKv2.
* Add support for testing multiple versions of the provider.
* Add `allowed_topologies` to the storageclass resource,
  authored by Ilia Lazebnik in PR
  hashicorp#910
* Fix acceptance tests and add upgrade tests.
dak1n1 added a commit that referenced this pull request Nov 4, 2020
@dak1n1
Update Terraform SDK to v2 (#1027)
aae2fef 
* Update to Terraform SDK v2.
* Update import paths and functions to use Terraform SDKv2.
* Add support for testing multiple versions of the provider.
* Add `allowed_topologies` to the storageclass resource,
  authored by Ilia Lazebnik in PR
  #910
* Fix acceptance tests and add upgrade tests.
@ghost
Copy link

ghost commented Nov 12, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. If you feel I made an error 🤖 🙉 , please reach out to my human friends 👉 hashibot-feedback@hashicorp.com. Thanks!

@ghost ghost locked as resolved and limited conversation to collaborators Nov 12, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@alexsomesan alexsomesan alexsomesan left review comments

@dak1n1 dak1n1 Awaiting requested review from dak1n1

Assignees
No one assigned
Labels
documentation size/XL
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Feature: allowed_topologies field in kubernetes storage class
3 participants
@DrFaust92 @dak1n1 @alexsomesan