Update CoreDNS manifests #878
Conversation
errordeveloper
force-pushed
the
fix-692
branch
3 times, most recently
from
d911c8c to
338d005
Compare
|
LGTM, 2 minor comments. Merge when you need to |
|
Would it be better to remove the field so if we ever forget to replace it
it will fail fast?
…On Thu, Jun 13, 2019, 18:01 Ilya Dmitrichenko ***@***.***> wrote:
***@***.**** commented on this pull request.
------------------------------
In pkg/addons/default/assets/coredns-1.11.json
<#878 (comment)>:
> + "metadata": {
+ "annotations": {
+ "prometheus.io/port": "9153",
+ "prometheus.io/scrape": "true"
+ },
+ "labels": {
+ "eks.amazonaws.com/component": "kube-dns",
+ "k8s-app": "kube-dns",
+ "kubernetes.io/cluster-service": "true",
+ "kubernetes.io/name": "CoreDNS"
+ },
+ "name": "kube-dns",
+ "namespace": "kube-system"
+ },
+ "spec": {
+ "clusterIP": "10.100.0.10",
No, and we handle here:
https://github.com/weaveworks/eksctl/pull/878/files#diff-9db8b5b22bd493bd608250a68252142fR211
—
You are receiving this because your review was requested.
Reply to this email directly, view it on GitHub
<#878>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAFT257223F2EPVTRR2EPMTP2JVMXANCNFSM4HXLKNHA>
.
|
|
@martina-if I've removed the field now. I kept the logic that sets |
This comment has been minimized.
This comment has been minimized.
|
This is good to go! |
| func UpdateCoreDNSImageTag(clientSet k8s.Interface, plan bool) (bool, error) { | ||
| printer := printers.NewJSONPrinter() | ||
| // UpdateCoreDNS will update the `coredns` add-on | ||
| func UpdateCoreDNS(rawClient kubernetes.RawClientInterface, region, controlPlaneVersion string, plan bool) (bool, error) { |
There was a problem hiding this comment.
There is a bunch of logic here now, it would be good to test that it is doing the right thing, not only not throwing an error.
| }) | ||
|
|
||
| It("can update based to latest version", func() { | ||
| _, err := UpdateCoreDNSImageTag(clientSet, false) | ||
| _, err := UpdateCoreDNS(rawClient, "eu-west-2", "1.12.x", false) |
There was a problem hiding this comment.
@martina-if what do you make of this test? It checks as much as currently possible with this fake client of ours...
There was a problem hiding this comment.
I suppose we could add tests with other kind of pre-conditions that would result in errors, if you think that's important?
There was a problem hiding this comment.
The problem I see is that if I break the logic in that function this test won't caught it because it only checks that there is no error returned. I prefer tests that tell you that you actually broke some piece of logic. Maybe it's hard to test with how it looks now. Perhaps it needs to be split in smaller pieces that can be tested. Or test that we call the client with the right information?
There was a problem hiding this comment.
See below, the test looks at what requests were made - does that seem sufficient?
There was a problem hiding this comment.
ok, I don't see a better way right now to test this 👍
Fix kustomize RBAC bindings to have namespace kube-system
Description
Close #692
Checklist
make build)make test)