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

Add freshpod as a new addon #2423

Merged
merged 2 commits into from Jan 23, 2018

Conversation

Projects
None yet
5 participants
@kairen
Contributor

kairen commented Jan 12, 2018

The commit added a new addon from #2420. This is very helpful for me to develop CRD operator on Minikube.

@minikube-bot

This comment has been minimized.

Collaborator

minikube-bot commented Jan 12, 2018

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/M label Jan 12, 2018

@kairen

This comment has been minimized.

Contributor

kairen commented Jan 12, 2018

/assign @r2d4
/cc @ahmetb

@k8s-ci-robot k8s-ci-robot requested a review from ahmetb Jan 12, 2018

@ahmetb

This comment has been minimized.

Member

ahmetb commented Jan 12, 2018

Haha I had the same patch sitting on my fork. :) Thanks @kairen.

# limitations under the License.
apiVersion: apps/v1
kind: Deployment

This comment has been minimized.

@ahmetb

ahmetb Jan 12, 2018

Member

can we make this a ReplicaSet, I believe we don't need Deployment’s rollout/rollback semantics here because add-on manager manages it exclusively.

This comment has been minimized.

@kairen

kairen Jan 12, 2018

Contributor

Yes, I have been changed to RC because other add-ons also use this kind of resource.

@@ -233,6 +233,13 @@ var Addons = map[string]*Addon{
"registry-creds-rc.yaml",
"0640"),
}, false, "registry-creds"),
"freshpod": NewAddon([]*BinDataAsset{
NewBinDataAsset(
"deploy/addons/freshpod/freshpod-dp.yaml",

This comment has been minimized.

@ahmetb

ahmetb Jan 12, 2018

Member

if you keep it as Deployment, -deploy might be a better suffix.

This comment has been minimized.

@kairen

kairen Jan 12, 2018

Contributor

No problem!! I'll change to RC.

template:
metadata:
labels:
version: v0.0.1

This comment has been minimized.

@ahmetb

ahmetb Jan 12, 2018

Member

it looks like we don't have a tagged version. I'll make sure to tag one and update the v0.0.1 image in-place.

This comment has been minimized.

@kairen

kairen Jan 12, 2018

Contributor

Okay, I have been removed from YAML file.

containers:
- name: freshpod
image: gcr.io/google-samples/freshpod:v0.0.1
imagePullPolicy: IfNotPresent

This comment has been minimized.

@ahmetb

ahmetb Jan 12, 2018

Member

Looks like other addons also use imagePullPolicy: IfNotPresent , I'm not sure why/if this is the preferred option. It doesn't matter much anyway.

This comment has been minimized.

@kairen

kairen Jan 13, 2018

Contributor

Yes, I'm referencing other addons to add this option, so I'm also not sure why this option to need because the addon doc not mention.

addonmanager.kubernetes.io/mode: Reconcile
template:
metadata:
labels:

This comment has been minimized.

@ahmetb

ahmetb Jan 12, 2018

Member

Do we need kubernetes.io/minikube-addons: freshpod here as well, so that the Pods get this label, too?
The documentation is unclear.

@ahmetb

This comment has been minimized.

Member

ahmetb commented Jan 12, 2018

This may be unrelated:

I checked our your branch and ran make out/minikube, then I tried to create a VM like ./out/minikube start --vm-driver=xhyve but I got this error:

...
Getting VM IP address...
Moving files into cluster...
E0112 11:18:32.269057   23784 start.go:229] Error updating cluster:  Error updating localkube from uri: Error creating localkube
asset from url: Error opening file asset: /Users/ahmetb/.minikube/cache/localkube/localkube-v1.9.0: open 
/Users/ahmetb/.minikube/cache/localkube/localkube-v1.9.0: no such file or directory
@kairen

This comment has been minimized.

Contributor

kairen commented Jan 13, 2018

Hi, @ahmetb If you want to build the latest version for Localkube, you need manually run make out/localkube to build it, and then copy the binary file from out/localkube to $HOME/.minikube/cache/localkube/localkube-v1.9.0, or use --bootstrapper to select other bootstrappers, such as kubeadm.

@ahmetb

This comment has been minimized.

Member

ahmetb commented Jan 14, 2018

/lgtm
thank you very much @kairen

@k8s-ci-robot

This comment has been minimized.

Contributor

k8s-ci-robot commented Jan 14, 2018

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: ahmetb, kairen
We suggest the following additional approver: r2d4

Assign the PR to them by writing /assign @r2d4 in a comment when ready.

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@ahmetb

This comment has been minimized.

Member

ahmetb commented Jan 14, 2018

/assign @r2d4

@r2d4

This comment has been minimized.

Member

r2d4 commented Jan 23, 2018

@minikube-bot ok to test

@r2d4

r2d4 approved these changes Jan 23, 2018

@r2d4

This comment has been minimized.

Member

r2d4 commented Jan 23, 2018

Thanks!

@r2d4 r2d4 merged commit ec1b443 into kubernetes:master Jan 23, 2018

7 of 15 checks passed

Minishift-Linux-KVM Jenkins
Details
Windows-Kubeadm-CRI-O Jenkins
Details
Windows-virtualbox Jenkins
Details
Linux-KVM Jenkins
Details
Linux-None Jenkins
Details
Linux-Virtualbox Jenkins
Details
Windows-Virtualbox Jenkins
Details
tide Not in tide pool.
Details
Jenkins Cross Build Build finished. No test results found.
Details
Linux-Container Jenkins
Details
Linux-VirtualBox Jenkins
Details
OSX-Hyperkit Jenkins
Details
OSX-Virtualbox Jenkins
Details
cla/linuxfoundation kairen authorized
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment