Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix pod template node_selectors #42

Merged
merged 2 commits into from
Aug 11, 2017

Conversation

sl1pm4t
Copy link
Contributor

@sl1pm4t sl1pm4t commented Aug 10, 2017

Prior to this fix the provider did not correctly set the nodeSelectors
attribute of a pod template.
This was because it silently failed the type conversion:
map[string]string != map[string]interface{}

Prior to this fix the provider did not correct set the nodeSelectors
attribute of a pod template. This was because it silently failed the 
type conversion ( map[string]string != map[string]interface{} ).
@radeksimko radeksimko added the bug label Aug 10, 2017
@radeksimko
Copy link
Member

Great spot, thanks for the fix. This looks good to me from functional perspective.

Would you mind attaching an acceptance test covering this particular fix - i.e. something like

resource "kubernetes_pod" "test" {
  metadata {
    name = "%s"
  }
  spec {
    container {
      image = "nginx"
      name  = "containername"
    }
    node_selector {
      "failure-domain.beta.kubernetes.io/region" = "%s"
    }
  }
}

The region can be a variable. We run all acceptance tests nightly on GKE and region is passed as ENV variable, so os.Getenv("GOOGLE_REGION") will do the job. The acceptance test would go into https://github.com/terraform-providers/terraform-provider-kubernetes/blob/master/kubernetes/resource_kubernetes_pod_test.go
and this one in particular is probably best as inspiration for "copy-paste-modify":

https://github.com/terraform-providers/terraform-provider-kubernetes/blob/master/kubernetes/resource_kubernetes_pod_test.go#L59-L80

Let me know if you have any questions regarding the testing suite or if you don't feel confident adding it, I can eventually do it for you - it may just take some extra time.

@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 10, 2017

Sure thing. I should be able to do that in the next day or so.

@sl1pm4t
Copy link
Contributor Author

sl1pm4t commented Aug 10, 2017

@radeksimko - I added the acc test:

go test -v -timeout 180s -tags  -run ^TestAccKubernetesPod_basic|TestAccKubernetesPod_importBasic|TestAccKubernetesPod_with_pod_security_context|TestAccKubernetesPod_with_container_liveness_probe_using_exec|TestAccKubernetesPod_with_container_liveness_probe_using_http_get|TestAccKubernetesPod_with_container_liveness_probe_using_tcp|TestAccKubernetesPod_with_container_lifecycle|TestAccKubernetesPod_with_container_security_context|TestAccKubernetesPod_with_volume_mount|TestAccKubernetesPod_with_cfg_map_volume_mount|TestAccKubernetesPod_with_resource_requirements|TestAccKubernetesPod_with_empty_dir_volume|TestAccKubernetesPod_with_nodeSelector$

=== RUN   TestAccKubernetesPod_basic
--- PASS: TestAccKubernetesPod_basic (27.38s)
=== RUN   TestAccKubernetesPod_importBasic
--- PASS: TestAccKubernetesPod_importBasic (6.29s)
=== RUN   TestAccKubernetesPod_with_pod_security_context
--- PASS: TestAccKubernetesPod_with_pod_security_context (5.83s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_exec
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_exec (41.64s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_http_get
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_http_get (14.36s)
=== RUN   TestAccKubernetesPod_with_container_liveness_probe_using_tcp
--- PASS: TestAccKubernetesPod_with_container_liveness_probe_using_tcp (6.25s)
=== RUN   TestAccKubernetesPod_with_container_lifecycle
--- PASS: TestAccKubernetesPod_with_container_lifecycle (6.08s)
=== RUN   TestAccKubernetesPod_with_container_security_context
--- PASS: TestAccKubernetesPod_with_container_security_context (6.45s)
=== RUN   TestAccKubernetesPod_with_volume_mount
--- PASS: TestAccKubernetesPod_with_volume_mount (23.03s)
=== RUN   TestAccKubernetesPod_with_cfg_map_volume_mount
--- PASS: TestAccKubernetesPod_with_cfg_map_volume_mount (8.16s)
=== RUN   TestAccKubernetesPod_with_resource_requirements
--- PASS: TestAccKubernetesPod_with_resource_requirements (5.73s)
=== RUN   TestAccKubernetesPod_with_empty_dir_volume
--- PASS: TestAccKubernetesPod_with_empty_dir_volume (6.40s)
=== RUN   TestAccKubernetesPod_with_nodeSelector
--- PASS: TestAccKubernetesPod_with_nodeSelector (5.24s)
PASS
ok  	github.com/terraform-providers/terraform-provider-kubernetes/kubernetes	162.927s
Success: Tests passed.

Thanks

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

@radeksimko radeksimko merged commit 2826a5e into hashicorp:master Aug 11, 2017
@sl1pm4t sl1pm4t deleted the fix-node-selectors branch August 11, 2017 08:12
@ghost ghost locked and limited conversation to collaborators Apr 22, 2020
# for free to subscribe to this conversation on GitHub. Already have an account? #.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants