Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update CoreDNS manifests #878

Merged
merged 7 commits into from Jun 17, 2019

Conversation

@errordeveloper
Copy link
Member

commented Jun 12, 2019

Description

Close #692

Checklist

  • Code compiles correctly (i.e make build)
  • Added tests that cover your change (if possible)
  • All unit tests passing (i.e. make test)

@errordeveloper errordeveloper marked this pull request as ready for review Jun 13, 2019

@errordeveloper errordeveloper force-pushed the fix-692 branch 3 times, most recently from d911c8c to 338d005 Jun 13, 2019

@errordeveloper errordeveloper requested a review from martina-if Jun 13, 2019

@martina-if

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

LGTM, 2 minor comments. Merge when you need to

@martina-if

This comment has been minimized.

Copy link
Member

commented Jun 13, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jun 13, 2019

@martina-if I've removed the field now. I kept the logic that sets metadata.resourceVersion and spec.clusterIP, as otherwise I'm seeing this error:

[✖]  Service "kube-dns" is invalid: [metadata.resourceVersion: Invalid value: "": must be specified for an update, spec.clusterIP: Invalid value: "": field is immutable]
@errordeveloper

This comment was marked as resolved.

Copy link
Member Author

commented Jun 13, 2019

I'd like to do some more manual tests on this tomorrow, as I didn't get a chance to try it in 1.11 –> 1.12 scenario.

@errordeveloper errordeveloper requested a review from martina-if Jun 14, 2019

@errordeveloper

This comment has been minimized.

Copy link
Member Author

commented Jun 14, 2019

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) {

This comment has been minimized.

Copy link
@martina-if

martina-if Jun 17, 2019

Member

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)

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Jun 17, 2019

Author Member

@martina-if what do you make of this test? It checks as much as currently possible with this fake client of ours...

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Jun 17, 2019

Author Member

I suppose we could add tests with other kind of pre-conditions that would result in errors, if you think that's important?

This comment has been minimized.

Copy link
@martina-if

martina-if Jun 17, 2019

Member

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?

This comment has been minimized.

Copy link
@errordeveloper

errordeveloper Jun 17, 2019

Author Member

See below, the test looks at what requests were made - does that seem sufficient?

This comment has been minimized.

Copy link
@martina-if

martina-if Jun 17, 2019

Member

ok, I don't see a better way right now to test this 👍

@errordeveloper errordeveloper merged commit 05ec5be into master Jun 17, 2019

3 checks passed

WIP Ready for review
Details
ci/circleci: make-eksctl-image Your tests passed on CircleCI!
Details
netlify/brave-ritchie-0c2cd8/deploy-preview Deploy preview ready!
Details

@errordeveloper errordeveloper deleted the fix-692 branch Jun 17, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.