Skip to content
This repository has been archived by the owner on Jun 12, 2024. It is now read-only.

draft init rewrite #154

Merged
merged 1 commit into from
Jun 21, 2017
Merged

Conversation

bacongobbler
Copy link
Contributor

@bacongobbler bacongobbler commented Jun 15, 2017

closes #82
closes #102
closes #122
closes #152
closes #167

This is a relatively "large" PR, so here's where we are at today:

  • Users can no longer update draft configuration through -f and --set; user will be prompted for this information instead if required
  • --yes is available if user wants to auto-accept the defaults
  • If "minikube" is detected as the current context in ~/.kube/config, then we assume that they have installed the ingress and registry addons as well as ran helm init after the cluster was created. draft init detects this and drops some configuration to use these addons natively

TODO:

  • prompt for missing information
  • update "Getting started" documentation

@msftclas
Copy link

@bacongobbler,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@bacongobbler
Copy link
Contributor Author

bit of a sample (with a custom minikube build with registry addon support):

$ minikube addons enable registry
$ minikube addons enable ingress
$ minikube start
$ helm init
$ draft init
Creating pack php...
Pack php already exists. Skipping!
Creating pack golang...
Pack golang already exists. Skipping!
Creating pack ruby...
Pack ruby already exists. Skipping!
Creating pack java...
Pack java already exists. Skipping!
Creating pack python...
Pack python already exists. Skipping!
Creating pack node...
Pack node already exists. Skipping!
$DRAFT_HOME has been configured at /Users/bacongobbler/.draft.
Draftd detected that you are using 'minikube' as your cloud provider. AWESOME!
Draftd is using the following configuration:

\``` [NOTE: backslashes added to escape triple backtick]
registry:
  url: $(REGISTRY_SERVICE_HOST)
  org: draft
basedomain: k8s.local
\```

Draftd has been installed into your Kubernetes Cluster.
Happy Sailing!

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Jun 16, 2017

testing this feature requires the following PR: kubernetes/minikube#1583

If you don't want to build minikube from scratch, you can just run

minikube addons enable ingress
minikube start --insecure-registry 10.0.0.0/24
kubectl create -f deploy/addons/registry/registry-rc.yaml
kubectl create -f deploy/addons/registry/registry-svc.yaml
helm init
draft init

As per the new Minikube quickstart documentation.

@bacongobbler
Copy link
Contributor Author

testers of this PR will also require kubernetes/minikube#1603. I accidentally broke the original PR on a last-minute fix.

@sgoings
Copy link
Member

sgoings commented Jun 19, 2017

What is your acceptance criteria for getting this merged? Any support for GKE/AWS k8s clusters (besides minikube)?

Right now if the provider isn't identified the messaging can be kind of confusing:

Creating pack java...
Pack java already exists. Skipping!
Creating pack python...
Pack python already exists. Skipping!
Creating pack golang...
Pack golang already exists. Skipping!
Creating pack node...
Pack node already exists. Skipping!
Creating pack php...
Pack php already exists. Skipping!
Creating pack ruby...
Pack ruby already exists. Skipping!
$DRAFT_HOME has been configured at /Users/sgoings/.draft.

Draftd detected that you are using  as your cloud provider.
Draftd would be using the following configuration:

<code block>
<code block>

Is this okay? (Y/n)

@sgoings
Copy link
Member

sgoings commented Jun 19, 2017

Also if I select "n" on that prompt, the ux doesn't feel quite right (draft moves on with changes instead of giving me cancel-like feedback):

Is this okay? (Y/n) n
Draftd has been installed into your Kubernetes Cluster.
Happy Sailing!

@bacongobbler
Copy link
Contributor Author

Yeah right now it's just minikube. Acceptance criteria for this PR is:

  • prompt for unknown cloud providers
  • update docs to reference the new QuickStart docs

Azure support would be a nice one to get in, but we can add support for that in a follow-up and just roll with minikube for now.

@bacongobbler
Copy link
Contributor Author

When you select "no" it should start to prompt you for manual information. That hasn't been implemented yet. Would that be ok to use or would you prefer a different UX?

@sgoings
Copy link
Member

sgoings commented Jun 19, 2017

I like that approach.

  • prompt for unknown cloud providers
  • a "no" starts prompting for manual information
  • doc update

Ping me when those changes go in! I like where this is headed.

@squillace
Copy link
Contributor

+1

@bacongobbler
Copy link
Contributor Author

bacongobbler commented Jun 19, 2017

I've revised the installation guide to make minikube front and center, as I imagine a lot of our userbase will be running a local cluster during development rather than on a cloud provider. I'll also add some bits about how any cluster can be used, but I haven't changed the codebase to prompt on unknown cloud providers yet to show how that works out.

@sgoings would you like to take a look at the installation guide and see if that works for you?

@bacongobbler
Copy link
Contributor Author

Something to note: this PR cannot be merged until minikube v0.20 has been released as the registry add-on is a new feature in master.

@@ -28,4 +28,4 @@ registry:
# For token-based logins, use
#
# $ echo '{"registrytoken":"9cbaf023786cd7"}' | base64
authtoken: changeme
authtoken: e30K
Copy link
Contributor

Choose a reason for hiding this comment

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

why this change? how come we need an auth token for pulling public images?

Copy link
Contributor Author

@bacongobbler bacongobbler Jun 20, 2017

Choose a reason for hiding this comment

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

"e30K" is the base64 equivalent of "{}". It's actually a very weird docker behaviour in which you NEED to send an empty auth token to push an image... even on registries with no authentication (like minikube w/ an in-cluster registry). Sending a string like "changeme" just causes docker to freak out because it can't parse it into a json object.

We could just change cmd/draft/installer/config/config.go to change the authtoken string to "e30K" but I thought it'd be a good default to use an empty JSON string anyways to prevent docker from freaking out.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See https://github.com/moby/moby/blob/254fc83cba90ed79c78f4cb0cb33aeeaff492798/client/image_push.go#L54 as an example. Even if the registry isn't backed by auth, an empty auth token still gets shipped and the v2 registry expects that.

@bacongobbler bacongobbler force-pushed the draft-init-rewrite branch 2 times, most recently from a944eb8 to 46bd465 Compare June 20, 2017 18:34
@bacongobbler bacongobbler changed the title [WIP] draft init rewrite draft init rewrite Jun 20, 2017
@bacongobbler bacongobbler force-pushed the draft-init-rewrite branch 2 times, most recently from bf50f79 to 6ddd759 Compare June 20, 2017 18:42
@bacongobbler
Copy link
Contributor Author

bacongobbler commented Jun 20, 2017

k, so the documentation has been rewritten with minikube in mind and the new "prompt for missing information" dialogue is available for users running on Azure/GCS/AWS/whathaveyou.

Sample output:

><> draft init
Creating pack java...
Pack java already exists. Skipping!
Creating pack node...
Pack node already exists. Skipping!
Creating pack php...
Pack php already exists. Skipping!
Creating pack ruby...
Pack ruby already exists. Skipping!
Creating pack python...
Pack python already exists. Skipping!
Creating pack golang...
Pack golang already exists. Skipping!
$DRAFT_HOME has been configured at /Users/bacongobbler/.draft.

Draft detected that you are using minikube as your cloud provider. AWESOME!
Draft will be using the following configuration:

'''
registry:
  url: $(REGISTRY_SERVICE_HOST)
basedomain: k8s.local
'''

Is this okay? [Y/n] nah

In order to install Draft, we need a bit more information...

1. Enter your Docker registry URL (e.g. docker.io, quay.io, myregistry.azurecr.io): docker.io
2. Enter your username: bacongobbler
3. Enter your password: 
4. Enter your org [bacongobbler]: 
5. Enter the top-level domain: draft.bacongobbler.com
Warning: Draft is already installed in the cluster.
Use --client-only to suppress this message.
Happy Sailing!

The password is hidden thanks to terminal.ReadPassword(). The organization also defaults to the username entered in step 2, but that can be changed as shown.

@bacongobbler
Copy link
Contributor Author

Some open questions:

  • "Enter the top-level domain" is not very descriptive to a new user. Any suggestions?
  • Similarly, "Enter your org". Maybe "Enter the docker org where draft will push images to (bacongobbler):"?

@codecov-io
Copy link

codecov-io commented Jun 20, 2017

Codecov Report

Merging #154 into master will decrease coverage by 0.09%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master     #154     +/-   ##
=========================================
- Coverage   33.15%   33.06%   -0.1%     
=========================================
  Files          18       18             
  Lines        1101     1104      +3     
=========================================
  Hits          365      365             
- Misses        687      690      +3     
  Partials       49       49
Impacted Files Coverage Δ
cmd/draft/installer/install.go 0% <ø> (ø) ⬆️
cmd/draft/init.go 2.45% <0%> (ø) ⬆️
api/server.go 15.35% <0%> (+0.05%) ⬆️
cmd/draft/draft.go 2.66% <0%> (-0.16%) ⬇️

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 e488d4a...c06bafa. Read the comment docs.

@bacongobbler
Copy link
Contributor Author

@bacongobbler bacongobbler merged commit a0c333f into Azure:master Jun 21, 2017
@bacongobbler bacongobbler deleted the draft-init-rewrite branch June 21, 2017 22:47
mboersma pushed a commit that referenced this pull request Jun 12, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.