Add crd #306
Add crd #306
Conversation
dturn
requested review from
KnVerey,
klautcomputing and
karanthukral
and removed request for
karanthukral
|
Looks good to me. Simple to start 👍 |
| if !exists? | ||
| super | ||
| elsif deploy_succeeded? | ||
| "Succeeded" |
There was a problem hiding this comment.
How about something more specific to what a CRD success means, like maybe "Registered" or even "Names accepted"?
There was a problem hiding this comment.
I wasn't sure if this was a place to be specific or match what other resources. Given the timeout_message "Names accepted" seems good to me
| end | ||
|
|
||
| def timeout_message | ||
| UNUSUAL_FAILURE_MESSAGE |
There was a problem hiding this comment.
Let's be more specific about what happened here, e.g. "The names this CRD is attempting to register were neither accepted nor rejected in time"
| # frozen_string_literal: true | ||
| module KubernetesDeploy | ||
| class CustomResourceDefinition < KubernetesResource | ||
| TIMEOUT = 30.seconds |
There was a problem hiding this comment.
I'd suggest being a little more generous here, since something actually does need to happen (vs. configmaps etc. where we're really just creating them and seeing that they showed up)
| @@ -94,5 +95,9 @@ def assert_daemon_set_up | |||
| def assert_stateful_set_up | |||
| assert_stateful_set_present("stateful-busybox") | |||
| end | |||
|
|
|||
| def assert_crd_up | |||
| assert_crd_present("mails.stable.example.io") | |||
There was a problem hiding this comment.
The mails plural still irks me :P
| assert_logs_match_all([ | ||
| "Deploying CustomResourceDefinition/mails.stable.example.io (timeout: 30s)", | ||
| "CustomResourceDefinition/mis-matched.stable.example.io: FAILED", | ||
| "Final status: ListKindConflict" |
There was a problem hiding this comment.
This is a little cryptic... Does the condition's message have additional information we're not printing?
There was a problem hiding this comment.
Yeah, I wasn't printing the message. I'll add it.
lib/kubernetes-deploy/deploy_task.rb
Outdated
| @@ -247,6 +248,10 @@ def discover_resources | |||
| @logger.info " - #{r.id}" | |||
| end | |||
| end | |||
| if (global = resources.select(&:global?).presence) | |||
| @logger.info("Detected non-namespaced #{'resouce'.pluralize(global.count)} which will never be pruned:") | |||
There was a problem hiding this comment.
let's print this in yellow, similar to these warnings:
| end | ||
|
|
||
| def test_crd_can_fail | ||
| assert_deploy_success(deploy_fixtures("hello-cloud", subset: ["crd.yml"])) |
There was a problem hiding this comment.
Is this first deploy necessary in this case? Won't mismatched names be rejected even on the first registration?
There was a problem hiding this comment.
We're actually creating a conflict between the listKind field in the fixture vs. the slightly modified fixture. A mismatch between the name & plural is caught at validation time and isn't really what we want to test.
dturn
force-pushed
the
add-crd
branch
2 times, most recently
from
3b0bb6b
to
38381fd
Compare
Because the CRD resource is global, I think the CRD test either need to create unique CRDs or not be run in parallel. I've gone with the unique route, we're polluting minikube, but I don't think that's a real issue. |
Could we not just delete the CRD at the end of each test run? |
We can't because of the parallelism. Test 1 runs, test 2 starts, test 1 finishes and deletes the CR. This could cause test 2 to succeeded when it shouldn't have or fail because in the middle of test 2 the CR is just gone. |
We have a file for tests that should not be run in parallel: |
|
I'm 👍 on the code and output now though. |
Add CRD Resource (part of splitting of https://github.com/Shopify/kubernetes-deploy/pull/188/files)
Success of failure based on the
NamesAcceptedstatus condition. Is not very sophisticated, but better than nothing.