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.

Sign up for GitHub

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

Jump to bottom

Add freshpod as a new addon #2423

Merged
merged 2 commits into from
Jan 23, 2018
Merged

Add freshpod as a new addon #2423

merged 2 commits into from
Jan 23, 2018

Conversation

kairen
Copy link
Contributor

@kairen 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.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 12, 2018
@minikube-bot
Copy link
Collaborator

Can one of the admins verify this patch?

@k8s-ci-robot k8s-ci-robot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jan 12, 2018
@kairen
Copy link
Contributor Author

kairen commented Jan 12, 2018

/assign @r2d4
/cc @ahmetb

@ahmetb
Copy link
Member

ahmetb commented Jan 12, 2018

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

ahmetb
ahmetb reviewed Jan 12, 2018
View reviewed changes
deploy/addons/freshpod/freshpod-dp.yaml Outdated
# limitations under the License.

apiVersion: apps/v1
kind: Deployment
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

pkg/minikube/assets/addons.go Outdated
@@ -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",
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No problem!! I'll change to RC.

deploy/addons/freshpod/freshpod-dp.yaml Outdated
template:
metadata:
labels:
version: v0.0.1
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I have been removed from YAML file.

ahmetb
ahmetb reviewed Jan 12, 2018
View reviewed changes
deploy/addons/freshpod/freshpod-rc.yaml
containers:
- name: freshpod
image: gcr.io/google-samples/freshpod:v0.0.1
imagePullPolicy: IfNotPresent
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

ahmetb
ahmetb reviewed Jan 12, 2018
View reviewed changes
deploy/addons/freshpod/freshpod-rc.yaml
addonmanager.kubernetes.io/mode: Reconcile
template:
metadata:
labels:
Copy link
Member

@ahmetb ahmetb Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

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

@ahmetb
Copy link
Member

ahmetb commented Jan 12, 2018 •
edited
Loading

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
Copy link
Contributor Author

kairen commented Jan 13, 2018 •
edited
Loading

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
Copy link
Member

ahmetb commented Jan 14, 2018

/lgtm
thank you very much @kairen

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 14, 2018
@k8s-ci-robot
Copy link
Contributor

[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
Copy link
Member

ahmetb commented Jan 14, 2018

/assign @r2d4

@r2d4
Copy link
Contributor

r2d4 commented Jan 23, 2018

@minikube-bot ok to test

@r2d4
Copy link
Contributor

r2d4 commented Jan 23, 2018

Thanks!

@r2d4 r2d4 merged commit ec1b443 into kubernetes:master Jan 23, 2018
@r2d4 r2d4 mentioned this pull request Jan 23, 2018
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahmetb ahmetb ahmetb left review comments

@r2d4 r2d4 r2d4 approved these changes

Assignees

@ahmetb ahmetb

@r2d4 r2d4

Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. lgtm "Looks good to me", indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@kairen @minikube-bot @ahmetb @k8s-ci-robot @r2d4