Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

handle new docker-desktop name #42

Closed
wants to merge 4 commits into from
Closed

handle new docker-desktop name #42

wants to merge 4 commits into from

Conversation

sbawaska
Copy link
Contributor

@sbawaska sbawaska commented Jun 4, 2019

replaces #37 by fixing unit test failures.
Also, replace the master server URL for docker desktop from localhost to host.docker.internal
so that it can be discovered from within the container executing CNAB install

closes #39.

@codecov
Copy link

codecov bot commented Jun 5, 2019

Codecov Report

Merging #42 into master will increase coverage by 0.41%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #42      +/-   ##
==========================================
+ Coverage   86.03%   86.44%   +0.41%     
==========================================
  Files          13       13              
  Lines         580      583       +3     
==========================================
+ Hits          499      504       +5     
+ Misses         54       53       -1     
+ Partials       27       26       -1
Impacted Files Coverage Δ
pkg/kab/patch_manifest.go 88.05% <100%> (+3.68%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 830de84...471ac6e. Read the comment docs.

Swapnil Bawaskar added 2 commits June 5, 2019 13:22
replace the master server URL for docker desktop from localhost to host.docker.internal
so that it can be discovered from within the container executing CNAB install
Copy link
Contributor

@scothis scothis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This level of sniffing and hacking will be very fragile. I'd rather we didn't sniff anything and instead expose the knobs necessary for a user to target the environment via documentation for their environment.

How does the CNAB community manage k8s configurations? I have to imagine the Docker folks have solved this.

@sbawaska
Copy link
Contributor Author

sbawaska commented Jun 5, 2019

I tried to come up with instructions first, but there is no good way to simplify the steps. The problem is outlined here: #39 (comment)
To make the cnab install work on docker-desktop, users will have to

  1. change ~/.kube/config to replace localhost with host.docker.internal
  2. run duffle install
  3. change back host.docker.internal to localhost in ~/.kube/config otherwise kubectl will not work ( since host.docker.internal is not resolvable from the host itself).

Then repeat the same steps for uninstall.

Also, if the user has watch kubectl get pods --all-namespaces running during install (as we have documented previously) that will now break, since ~/.kube/config is pointing at host.docker.internal which is not resolvable from host.

If the user misses any of the steps outlined, they are going to have problems installing OR running kubectl after install. I feel that the above prescriptive flow for our users is more brittle than us trying to patch kubeconfig.

@scothis
Copy link
Contributor

scothis commented Jun 6, 2019

To make the cnab install work on docker-desktop, users will have to

Yea, we don't want users to manually edit the kube config.

We can do better by either:

  • allowing the user to specify the k8s api server host as a config input, using the kube config value as a default
  • encourage one-off kubeconfig files
  • other mechanisms to authenticate to the api server

We can figure out what the right balance of config options.

@scothis
Copy link
Contributor

scothis commented Jun 6, 2019

Another option, run the invocation image inside the k8s cluster. This way we don’t need a local docker daemon.

@glyn
Copy link
Contributor

glyn commented Jun 6, 2019

How does the CNAB community manage k8s configurations? I have to imagine the Docker folks have solved this.

I'm investigating this.

@glyn
Copy link
Contributor

glyn commented Jun 7, 2019

docker-app patches the kube config for docker desktop when creating a CNAB from a docker application package. However, they hit the same problem as duffle when using an CNAB which has not undergone this patching.

@sbawaska
Copy link
Contributor Author

We now run the invocationImage in cluster, hence this is not longer applicable.

@sbawaska sbawaska closed this Jun 25, 2019
@sbawaska sbawaska deleted the docker-desktop branch June 25, 2019 17:52
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Error installing on Docker Desktop
3 participants