Add support for kubernetes_replication_controller #9

radeksimko · 2017-06-20T15:06:36Z

Test results

make testacc TESTARGS='-run=TestAccKubernetesReplicationController_'
==> Checking that code complies with gofmt requirements...
TF_ACC=1 go test $(go list ./... |grep -v 'vendor') -v -run=TestAccKubernetesReplicationController_ -timeout 120m
?   	github.com/terraform-providers/terraform-provider-kubernetes	[no test files]
=== RUN   TestAccKubernetesReplicationController_basic
--- PASS: TestAccKubernetesReplicationController_basic (123.60s)
=== RUN   TestAccKubernetesReplicationController_importBasic
--- PASS: TestAccKubernetesReplicationController_importBasic (120.65s)
=== RUN   TestAccKubernetesReplicationController_generatedName
--- PASS: TestAccKubernetesReplicationController_generatedName (4.89s)
=== RUN   TestAccKubernetesReplicationController_importGeneratedName
--- PASS: TestAccKubernetesReplicationController_importGeneratedName (5.14s)
=== RUN   TestAccKubernetesReplicationController_with_security_context
--- PASS: TestAccKubernetesReplicationController_with_security_context (4.71s)
=== RUN   TestAccKubernetesReplicationController_with_container_liveness_probe_using_exec
--- PASS: TestAccKubernetesReplicationController_with_container_liveness_probe_using_exec (6.14s)
=== RUN   TestAccKubernetesReplicationController_with_container_liveness_probe_using_http_get
--- PASS: TestAccKubernetesReplicationController_with_container_liveness_probe_using_http_get (4.51s)
=== RUN   TestAccKubernetesReplicationController_with_container_liveness_probe_using_tcp
--- PASS: TestAccKubernetesReplicationController_with_container_liveness_probe_using_tcp (5.54s)
=== RUN   TestAccKubernetesReplicationController_with_container_lifecycle
--- PASS: TestAccKubernetesReplicationController_with_container_lifecycle (5.47s)
=== RUN   TestAccKubernetesReplicationController_with_container_security_context
--- PASS: TestAccKubernetesReplicationController_with_container_security_context (5.38s)
=== RUN   TestAccKubernetesReplicationController_with_volume_mount
--- PASS: TestAccKubernetesReplicationController_with_volume_mount (6.78s)
=== RUN   TestAccKubernetesReplicationController_with_resource_requirements
--- PASS: TestAccKubernetesReplicationController_with_resource_requirements (3.23s)
=== RUN   TestAccKubernetesReplicationController_with_empty_dir_volume
--- PASS: TestAccKubernetesReplicationController_with_empty_dir_volume (3.51s)
PASS
ok  	github.com/terraform-providers/terraform-provider-kubernetes/kubernetes	299.632s

apparentlymart

I don't feel qualified to review this in detail due to my limited knowledge of Kubernetes, but the implementation looks plausible and I just had some minor feedback on the docs and naming.

apparentlymart · 2017-06-20T15:52:46Z

website/docs/r/replication_controller.html.markdown

+          }
+          requests{
+            cpu = "250m"
+            memory = "50Mi"


we should align these by equals to match what hclfmt would do.

apparentlymart · 2017-06-20T15:52:52Z

website/docs/r/replication_controller.html.markdown

+    selector {
+      Test = "MyExampleApp"
+    }
+  	template {


indentation seems wrong here

apparentlymart · 2017-06-20T15:58:36Z

website/docs/r/replication_controller.html.markdown

+* `env` - (Optional) List of environment variables to set in the container. Cannot be updated.
+* `image` - (Optional) Docker image name. More info: http://kubernetes.io/docs/user-guide/images
+* `image_pull_policy` - (Optional) Image pull policy. One of Always, Never, IfNotPresent. Defaults to Always if :latest tag is specified, or IfNotPresent otherwise. Cannot be updated. More info: http://kubernetes.io/docs/user-guide/images#updating-images
+* `lifecycle` - (Optional) Actions that the management system should take in response to container lifecycle events


Of course at this nested level lifecycle isn't special but since we have a special meaning of it at the resource level, should we try to pick a non-conflicting name here in case we want to mimic this same nested structure as a top-level resource later?

Probably not a big deal unless we can already imagine a situation where a container structure like this would be a top-level resource. So feel free to ignore if you disagree. 😀

ah 🤔 I see what do you mean, but I'd rather keep it as is. Vast majority of the code + docs was generated based on K8S API structs and I think it's good that we stick to their naming conventions where possible.

I know it may cause troubles if we ever get to implement the nested resources - as 1st level meta-params might suddenly become valid in nested structures, but I'd prefer to deal with it when it becomes a real problem.

catsby

2(?) questions but non-blockers I think 👍

catsby · 2017-06-21T20:47:44Z

website/docs/r/replication_controller.html.markdown

+The following arguments are supported:
+
+* `metadata` - (Required) Standard replication controller's metadata. More info: https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md#metadata
+* `spec` - (Required) Spec defines the specification of the desired behavior of the replication controller. More info: http://releases.k8s.io/HEAD/docs/devel/api-conventions.md#spec-and-status


https://github.com/kubernetes/kubernetes/blob/ee89c30eb3b6945f450b35926a94ab66ff1c4401/docs/devel/api-conventions.md#spec-and-status reports "this file has moved" -> https://github.com/kubernetes/community/blob/master/contributors/devel/api-conventions.md

Good catch, this is kind of a side effect of K8S having these outdated links in swagger/API specs from which I've generated the schema and then these docs. 😢

I'll fix it here manually and then sed the existing docs too in a separate PR.

catsby · 2017-06-21T21:29:22Z

kubernetes/resource_kubernetes_replication_controller.go

+		return fmt.Errorf("Failed to update replication controller: %s", err)
+	}
+	log.Printf("[INFO] Submitted updated replication controller: %#v", out)
+	d.SetId(buildId(out.ObjectMeta))


Is it normal for the ID of the resource to change here? Does the old ID still work? Is it possible for the Patch operation above (~line 170) to encounter an error but still actually change the id, in which case something is lost?

If this kind of id updating is normal then ignore me. I've seen other resources re-set the ID in UPDATE methods when it wasn't necessary (or recommended)

Ah, you're absolutely right, thanks for pointing it out. Neither name or namespace (from which we build the ID) are updatable, so the ID would never change. Fixed.

radeksimko · 2017-06-22T06:52:22Z

Both fixed. Thanks for the review.

radeksimko added the new-resource label Jun 20, 2017

apparentlymart reviewed Jun 20, 2017

View reviewed changes

radeksimko force-pushed the f-replication-controller branch from 371d81d to 895062e Compare June 21, 2017 10:09

catsby approved these changes Jun 21, 2017

View reviewed changes

Add support for kubernetes_replication_controller

e86504a

radeksimko force-pushed the f-replication-controller branch from 895062e to e86504a Compare June 22, 2017 06:48

radeksimko merged commit 59fc241 into master Jun 22, 2017

radeksimko deleted the f-replication-controller branch June 22, 2017 06:52

radeksimko mentioned this pull request Jun 22, 2017

docs: Fix outdated links to K8S docs #10

Merged

ghost locked and limited conversation to collaborators Apr 22, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add support for kubernetes_replication_controller #9

Add support for kubernetes_replication_controller #9

radeksimko commented Jun 20, 2017

apparentlymart left a comment

apparentlymart Jun 20, 2017

apparentlymart Jun 20, 2017

apparentlymart Jun 20, 2017

radeksimko Jun 21, 2017

catsby left a comment

catsby Jun 21, 2017

radeksimko Jun 22, 2017

catsby Jun 21, 2017

radeksimko Jun 22, 2017

radeksimko commented Jun 22, 2017

Add support for kubernetes_replication_controller #9

Add support for kubernetes_replication_controller #9

Conversation

radeksimko commented Jun 20, 2017

Test results

apparentlymart left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

catsby left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

radeksimko commented Jun 22, 2017