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.

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

Addon: add headlamp #7

Closed
wants to merge 1 commit into from
Closed

Addon: add headlamp #7

wants to merge 1 commit into from

Conversation

yolossn
Copy link
Member

@yolossn yolossn commented Jun 2, 2022

Signed-off-by: Santhosh Nagaraj S [email protected]

Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

Awesome.

I didn't try it yet (I will in a while!). Been looking forward to using this :)

  • Where is the description of the addon? Probably this needs to be added somewhere.
  • Why would this be added as well as the dashboard addon? Probably write that it's extensible dashboard.

go.mod Outdated Show resolved Hide resolved
pkg/addons/config.go Outdated Show resolved Hide resolved
Copy link
Member

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

Pretty cool! What would be the commands for users to be able to expose/access Headlamp in the browser? Have you checked whether we can expose an endpoint, so we can minikube addons open headlamp?

I'm not sure this is possible but my idea is that new k8s users could set up Headlamp and access it in the browser without any prior knowledge of how to expose services in k8s.

cmd/minikube/cmd/config/enable.go Outdated Show resolved Hide resolved
cmd/minikube/cmd/config/enable.go Show resolved Hide resolved
cmd/minikube/cmd/config/enable.go Outdated Show resolved Hide resolved
@yolossn yolossn force-pushed the headlamp_addon branch 2 times, most recently from 5700408 to 2f6af6d Compare June 6, 2022 07:18
@yolossn
Copy link
Member Author

yolossn commented Jun 6, 2022

  • Where is the description of the addon? Probably this needs to be added somewhere.

The description should be added in the upstream PR.

  • Why would this be added as well as the dashboard addon? Probably write that it's extensible dashboard.

Will include this in the upstream PR description 👍🏽

@illume
Copy link
Member

illume commented Jun 6, 2022

The description should be added in the upstream PR.

Cool.

A few other things for your consideration:

  1. I found this page about addons: https://minikube.sigs.k8s.io/docs/contrib/addons/ I guess you saw it already? There's some things about testing it in there. I'm not sure if there's some other tests we need to add for the addon? There is test/integration/addons_test.go which has integration tests for other addons.
  2. I saw this PR review process posted by bots into other Minikube PRs. Probably worth checking out if you haven't seen it already.
  3. Addons can have a documentation page in this path: https://github.com/kubernetes/minikube/tree/master/site/content/en/docs/handbook/addons I suggest adding one to the PR. Maybe the structure of these other addon docs (and the dashboard docs) could be used to guide documentation for this addon.
  4. The dashboard addon documentation had something interesting... it had a --url option. Perhaps we could also have that to match the dashboard addon?
  5. I saw this upcoming PR Add doc URL to the addon list table kubernetes/minikube#14123 which might require changes from this PR (adding a link to the docs).
  6. Some PRs for other merged addons: kong addon [Addon] Kong Ingress Controller kubernetes/minikube#13326), portainer, CSI Hostpath Driver & VolumeSnapshots addons. There are some unmerged addons too.
  7. This upcoming security scanning PR is interesting. They are going to be testing addon images for reported security issues.

@yolossn yolossn force-pushed the headlamp_addon branch 4 times, most recently from 47f46c6 to afae655 Compare June 8, 2022 15:14
@@ -0,0 +1,41 @@
## Headlamp Addon
[Headlamp](https://github.com/kinvolk/headlamp) - Headlamp is an easy-to-use and extensible Kubernetes web UI.
Copy link
Member Author

Choose a reason for hiding this comment

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

Have added documentation for the addon.

Copy link
Member

Choose a reason for hiding this comment

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

BTW, why not just add the link + the description: Headlamp is an easy-to-use ....

Copy link
Member

Choose a reason for hiding this comment

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

I have also noticed that apparently other addons don't have a README.md file, so not sure if they will accept it for us.

@yolossn
Copy link
Member Author

yolossn commented Jun 8, 2022

  1. Addons can have a documentation page in this path: https://github.com/kubernetes/minikube/tree/master/site/content/en/docs/handbook/addons I suggest adding one to the PR. Maybe the structure of these other addon docs (and the dashboard docs) could be used to guide documentation for this addon.

Done

  1. The dashboard addon documentation had something interesting... it had a --url option. Perhaps we could also have that to match the dashboard addon?

This looks custom made for the dashboard command, I don't think something like this is possible unless we propose a headlamp command.

  1. I saw this upcoming PR Add doc URL to the addon list table kubernetes/minikube#14123 which might require changes from this PR (adding a link to the docs).

Great, have added README for headlamp too 👍🏽

  1. Some PRs for other merged addons: kong addon [Addon] Kong Ingress Controller kubernetes/minikube#13326), portainer, CSI Hostpath Driver & VolumeSnapshots addons. There are some unmerged addons too.

👍🏽

  1. This upcoming security scanning PR is interesting. They are going to be testing addon images for reported security issues.

Interesting!

Copy link
Member

@joaquimrocha joaquimrocha left a comment

Choose a reason for hiding this comment

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

It's looking very good. I found some issues related to language, so left some comments.
Also, I see you added the volume claim but I haven't seen instructions on how to set up the plugins. If the instructions should be in headlamp's docs, maybe we shouldn't have the volume claim here and just have instructions on how to set up plugins in a generic (in-cluster) way and point minikube's users there on a successful addon enabling.

cmd/minikube/cmd/config/enable.go Outdated Show resolved Hide resolved
Comment on lines +80 to +81
export SECRET=$(kubectl get secrets --namespace headlamp -o custom-columns=":metadata.name" | grep "headlamp-token")
kubectl get secret $SECRET --namespace headlamp --template=\{\{.data.token\}\} | base64 --decode
Copy link
Member

Choose a reason for hiding this comment

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

This is slightly different from what we have as instructions in https://kinvolk.github.io/headlamp/docs/latest/installation/#create-a-service-account-token .
Should be make more similar? (the current way looks good, just wonder if there's a way to approximate either case)

Copy link
Member Author

Choose a reason for hiding this comment

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

I think it makes sense to update the documentation as it involves the user to copy the headlamp-token name, whereas this solution can be run directly.

deploy/addons/headlamp/README.md Outdated Show resolved Hide resolved
deploy/addons/headlamp/README.md Outdated Show resolved Hide resolved
cmd/minikube/cmd/config/enable.go Outdated Show resolved Hide resolved
deploy/addons/headlamp/README.md Outdated Show resolved Hide resolved
deploy/addons/headlamp/README.md Outdated Show resolved Hide resolved
@@ -0,0 +1,41 @@
## Headlamp Addon
[Headlamp](https://github.com/kinvolk/headlamp) - Headlamp is an easy-to-use and extensible Kubernetes web UI.
Copy link
Member

Choose a reason for hiding this comment

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

BTW, why not just add the link + the description: Headlamp is an easy-to-use ....

@@ -0,0 +1,41 @@
## Headlamp Addon
[Headlamp](https://github.com/kinvolk/headlamp) - Headlamp is an easy-to-use and extensible Kubernetes web UI.
Copy link
Member

Choose a reason for hiding this comment

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

I have also noticed that apparently other addons don't have a README.md file, so not sure if they will accept it for us.

site/content/en/docs/handbook/addons/headlamp.md Outdated Show resolved Hide resolved
@illume
Copy link
Member

illume commented Jun 8, 2022

The dashboard addon documentation had something interesting... it had a --url option. Perhaps we could also have that to match the dashboard addon?

This looks custom made for the dashboard command, I don't think something like this is possible unless we propose a headlamp command.

Seems to be some other optional addons have a command. Like this gvisor one: kubernetes#3399 In perhaps a future PR I think it's worth asking about adding an optional headlamp cmd... perhaps on a future PR. Being able to run code could be useful for making some of these things run as smoothly as minkube dashboard.

ps. The I noticed the service command works with --url similar to the minikube dashboard --url.

@yolossn yolossn force-pushed the headlamp_addon branch 2 times, most recently from a729874 to cd455e5 Compare June 9, 2022 04:27
Copy link
Member

@illume illume left a comment

Choose a reason for hiding this comment

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

lgtm 🎈🎉

Signed-off-by: Santhosh Nagaraj S <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants