fix: add an override for Job in cluster resource discovery #696
Conversation
sco11morgan
requested review from
DazWorrall,
dturn,
KnVerey and
a team
as code owners
|
@dturn any idea why buildkite isn't kicking off for my PR? |
For security reasons, a Shopify employee has to manually trigger it on PRs from external contributors. Thanks for the ping, I'll kick it off shortly. |
|
Test passed, the red x is due to a CI limitation on our side. |
| @@ -50,7 +50,7 @@ def test_prunable_namespaced_resources | |||
| kinds = crd.prunable_resources(namespaced: true) | |||
|
|
|||
| assert_equal(kinds.length, 25) | |||
| %w(ConfigMap CronJob Deployment).each do |expected_kind| | |||
| %w(ConfigMap CronJob Deployment batch/v1/Job).each do |expected_kind| | |||
There was a problem hiding this comment.
Shouldn't this just be "Job"? Otherwise of course L54 will only ever find one matching entry, before or after this PR. I guess the matcher needs to get more specific than just include? since it will also match "CronJob".
There was a problem hiding this comment.
@KnVerey the point of the test is to assert that the version override is working and a specific Job API is returned ("v1" not v1beta1 or v2alpha1)
There was a problem hiding this comment.
@sco11morgan The name of this test is vague, it was originally designed to ensure that a few obvious Kinds were present. How would you feel about adding an additional block to test for full group/version/kind matches:
%w(batch/v1/Job batch/v1beta1/cronjob).each do |expected_gvk|
assert kinds.include?(expected_gvk)
end
There was a problem hiding this comment.
👍 moved the assertions to its own test
* fix: add an override for Job in cluster resource discovery * separate assertions for group/version/kind overrides
What are you trying to accomplish with this PR?
...
fix the error
no matches for kind "Job" in version "batch/v2alpha1"#687How is this accomplished?
adding an override for Job in cluster resource discovery
What could go wrong?
...