ServiceAccount resources should be predeployed like ConfigMaps #221
Conversation
| @@ -116,6 +116,7 @@ def create_or_update_secret(secret_name, secret_type, data) | |||
|
|
|||
| secret_yaml = generate_secret_yaml(secret_name, secret_type, data) | |||
| file = Tempfile.new(secret_name) | |||
|
|
|||
There was a problem hiding this comment.
Spurious newline - let's aim for a minimal change set.
| end | ||
| # Expect deploying the unmanaged pod fails due to missing service account | ||
| assert_deploy_failure(result) | ||
| # if KUBE_CLIENT_VERSION < Gem::Version.new("1.8.0") |
There was a problem hiding this comment.
Is this a leftover from testing?
| @@ -27,6 +27,51 @@ def test_full_hello_cloud_set_deploy_succeeds | |||
| assert_logs_match(%r{ConfigMap/hello-cloud-configmap-data\s+Available}, 1) | |||
| end | |||
|
|
|||
| def test_service_account_predeployed_before_unmanaged_pod | |||
| # Add an undefined service account in unmanaged pod | |||
| service_account_name = "unknown-service-account" | |||
There was a problem hiding this comment.
Is the first part of this test adding much value?
The failure here comes from Kubernetes - we know the deployment will fail if it mentions a missing service account, so we're not testing k8s-deploy specific functionality. Unless I'm missing something, I'd suggest deleting this test case and keeping only the success case.
There was a problem hiding this comment.
I agree with @stefanmb on this.
There was a problem hiding this comment.
Agree. Removed the first part of the test which does not test feature provided kubernetes-deploy
| @@ -27,6 +27,51 @@ def test_full_hello_cloud_set_deploy_succeeds | |||
| assert_logs_match(%r{ConfigMap/hello-cloud-configmap-data\s+Available}, 1) | |||
| end | |||
|
|
|||
| def test_service_account_predeployed_before_unmanaged_pod | |||
| # Add an undefined service account in unmanaged pod | |||
| service_account_name = "unknown-service-account" | |||
There was a problem hiding this comment.
I agree with @stefanmb on this.
| @@ -0,0 +1,7 @@ | |||
| apiVersion: v1 | |||
There was a problem hiding this comment.
I don't think this new service account, and by extension your image pull secret, is ever being used, since your test uses the hello-cloud fixture set, not the ejson-cloud set. If you switch your test to use ejson-cloud, one option would be to make ejson-cloud's web.yaml always mount this new service account and have your test assert that it does so.
There was a problem hiding this comment.
Added another test "test_pod_get_image_pull_secret_from_service_account" in ejson-cloud to verify that if a pod does not contain any ImagePullSecrets, ImagePullSecrets of the pod's ServiceAccount should be added to the pod.
Test "test_service_account_predeployed_before_unmanaged_pod" in hello_cloud is to verify that service accounts are deployed before unmanaged pods.
There was a problem hiding this comment.
Isn't that basically just testing a kubernetes feature again? That's not something the gem is doing itself.
There was a problem hiding this comment.
Following our discuss, I have removed the test for image pull secret.
#179
Force deploy service accounts before unmanaged pods in redeployment.
Tested a service account is deployed before a pod that refers to it.
Tested that an image pull secret that a service account refers is deployed before all redeployed resources.