Kind: Umbrella Issue: Code Quality Improvements

[BenTheElder]

What should be cleaned up or changed:

• [x] enable staticcheck and fix errors https://staticcheck.io/docs/
• [x] factor out all exec / docker interaction into interfaces for mocking and implementing against podman etc.
  
  
  
  • [x] eliminate the caching mechanism on node handles, instead provide a caching wrapper for this docker interface, and only use it by default in the CLI (library users should need to opt into caching since we don't know how long lived their processes are and we probably won't have good invalidation)
  
  
• [x] unit tests, as the code grows and matures, we should write more of them! [WIP]
• [x] [shellcheck](https://www.shellcheck.net/) our bash. it's not going anywhere for the little dev scripts, and we run a tiny bit in the node entrypoint now. we should ensure this is high quality. we can create a simpler variation on my kubernetes/kubernetes shellcheck script.

Why is this needed:
To ensure kind's code improves in quality and stays that way.

/priority important-longterm
/assign

Will work on this more after the 0.3 release. Plan to go ahead and get some smaller pieces in while some tests soak.

[aojea]

@BenTheElder should we add some e2e smoke tests for features like the load balancer and port mapping?

[BenTheElder]

@aojea IE HA mode?

Eventually yes, but lower priority, to my knowledge nothing depends on that yet. (And shouldn't!)

We also should have smoke tests with the default image.

[nzoueidi]

I might help with the two last points :)

[alisondy]

another one for the list potentially?
Hadolint our docker files see here

[BenTheElder]

584 and #591 added a shellcheck linter and eliminated failures (except the one lint we turn off regarding command -v vs which)

[glerchundi]

Is it possible to avoid the dependency with docker binary by vendoring the client library and making the calls directly to the API instead of executing them through exec?

[BenTheElder]

Good question.

It's pretty wildly unusual to have a docker daemon running but no docker client binary so we're not trying to avoid depending on the binary, however we are trying to let kind work with other runtimes (podman).

(Also the docker client library
is pretty terrible, kubelet is littered with comments about trying to figure out how to use it correctly. Users will have a much better time debugging when they can see the CLI calls by adjusting the log level)

[glerchundi]

Sounds reasonable but there could be a common use case where you are
running kind inside a container that has access to a docker daemon for CI
purposes.

Just putting my 2 cents.

Great job by the way 👍🏻

On Thu, 6 Jun 2019 at 18:22, Benjamin Elder [e-mail]
wrote:

Good question.

It's pretty wildly unusual to have a docker daemon running but no docker
client binary so we're not trying to avoid depending on the binary, however
we are trying to let kind work with other runtimes (podman).

(Also the docker client library
is pretty terrible, kubelet is littered with comments about trying to
figure out how to use it correctly. Users will have a much better time
debugging when they can see the CLI calls by adjusting the log level)

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/kubernetes-sigs/kind/issues/528?email_source=notifications&email_token=AARA7FW4YEPWA7GTUIN2U2DPZE2SXA5CNFSM4HM6VDEKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXDMUQY#issuecomment-499567171,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AARA7FWY3EU4QDFSC75AOL3PZE2SXANCNFSM4HM6VDEA
.

[BenTheElder]

Small update: we've added golangci-lint with a number of extra linters enabled across the repo and fixed ~all of the failures.

[BenTheElder]

we're mostly decoupled from docker, and span up some more effort on testing, including collecting coverage data and soon e2e coverage too.

[BenTheElder]

factoring out docker into a provider and node interface is 100% done for cluster operations.

image building I don't think we want to tackle for now, I'd rather only maintain one build, but we can see if s/docker/$tool/ works for any tools implementing the same build command maybe..

we definitely want more unit tests, and are going to collect e2e test coverage, but we do have a lot more unit tests now and are working to improve coverage of stable behaviors (like kubeconfig management).

[BenTheElder]

https://prow.k8s.io/view/gcs/kubernetes-jenkins/logs/ci-kind-unit/1190267720940130305

We're up to 22% of statements and 31% of files.

Ordinarily I'd be very unhappy with that, but the other code is _mostly_ things like creating the nodes with docker exec, which we e2e test extensively.

I have a WIP PR to collect the e2e coverage of kind, we can diff when that's in and fill the gaps.

This code still has other quality improvements to come, but the ones in the initial comment are in much better shape.