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

Operator for the Kubernetes dashboard #44

Merged
merged 8 commits into from
Mar 17, 2020
Merged

Operator for the Kubernetes dashboard #44

merged 8 commits into from
Mar 17, 2020

Conversation

somtochiama
Copy link
Member

This is a pull request to add an operator for the Kubernetes dashboard

@k8s-ci-robot k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Feb 28, 2020
@atoato88
Copy link
Contributor

atoato88 commented Mar 3, 2020

@somtochiama
Thank you for creating PR 😄
dashboard-operator exists at kubebuilder-declarative-pattern repo as a example of kubebuilder-declarative-pattern.
Recently, we discussed about move dashboard-operator from kubebuilder-declarative-pattern repo to cluster-addons repo.
So your PR is very good for me.
Could you see kubebuilder-declarative-pattern repo and mix to your PR?

/cc @johnsonj

@k8s-ci-robot k8s-ci-robot requested a review from johnsonj March 3, 2020 01:34
@somtochiama
Copy link
Member Author

discussed

@somtochiama
Thank you for creating PR 😄
dashboard-operator exists at kubebuilder-declarative-pattern repo as a example of kubebuilder-declarative-pattern.
Recently, we discussed about move dashboard-operator from kubebuilder-declarative-pattern repo to cluster-addons repo.
So your PR is very good for me.
Could you see kubebuilder-declarative-pattern repo and mix to your PR?

/cc @johnsonj

Sure. I will do that.

@atoato88
Copy link
Contributor

atoato88 commented Mar 3, 2020

@somtochiama
IMHO, the difference of between your PR and dashboard-operator in kubebuilder-declarative-pattern is following.

  • dashboard-operator in kubebuilder-declarative-pattern has tests for operator at here
  • dashboard-operator in kubebuilder-declarative-pattern registers more functions in Reconciler.Init
  • dashboard-operator in kubebuilder-declarative-pattern has supports for some versions of dashboard.

@johnsonj
Should we move the code of dashboard-operator in kubebuilder-declarative-pattern to cluster-addons first, and next, modify the code if any updates?
WDYT?

@atoato88
Copy link
Contributor

atoato88 commented Mar 3, 2020

Thank you to update!
I have single comment.

  • Copyright missing at dashboard/controllers/dashboard_controller.go and dashboard/api/v1alpha1/dashboard_types.go

@atoato88
Copy link
Contributor

atoato88 commented Mar 4, 2020

Could you add tests?

  • dashboard-operator in kubebuilder-declarative-pattern has tests for operator at here

atoato88
atoato88 reviewed Mar 11, 2020
View reviewed changes
Copy link
Contributor

@atoato88 atoato88 left a comment

Choose a reason for hiding this comment

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

@somtochiama
Thank you for updating PR.
Almost OK for me.

@johnsonj
There is a question about maintainers key in Application CR.
Please see PR comments.

dashboard/config/crd/kustomization.yaml
# since it depends on service name and namespace that are out of this kustomize package.
# It should be run by config/default
resources:
- bases/addons.k8s.io_dashboards.yaml
Copy link
Contributor

Choose a reason for hiding this comment

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

Add setting for building manifests of Application CR because dashboard operator use it.

Suggested change
- bases/addons.k8s.io_dashboards.yaml
- bases/addons.k8s.io_dashboards.yaml
- bases/app.k8s.io_application.yaml

dashboard/channels/packages/dashboard/1.10.1/manifest.yaml
descriptor:
type: "kubernetes-dashboard"
description: |
This is a demo of the kubebuilder-declarative-pattern project and not a production ready installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

Dashboard operator is not demo of the kubebuilder-declarative-pattern no longer.
We can delete this.

Suggested change
This is a demo of the kubebuilder-declarative-pattern project and not a production ready installation.

dashboard/channels/packages/dashboard/1.10.1/manifest.yaml Outdated
type: "image/png"
maintainers:
- name: Maintainer
email: [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

@johnsonj
What value should we change with?
This value is [email protected] since v1.8.3 in kubebuilder-declarative-pattern

Or, can we change here with below?

Suggested change
email: [email protected]
url: https://github.com/kubernetes/dashboard

dashboard/channels/packages/dashboard/1.8.3/manifest.yaml
type: "kubernetes-dashboard"
description: |
This is a demo of the kubebuilder-declarative-pattern project and not a production ready installation.
Kubernetes Dashboard is a general purpose, web-based UI for Kubernetes clusters.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

dashboard/channels/packages/dashboard/1.8.3/manifest.yaml Outdated
type: "image/png"
maintainers:
- name: Maintainer
email: [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

dashboard/channels/packages/dashboard/2.0.0/manifest.yaml Outdated
type: "image/png"
maintainers:
- name: Maintainer
email: [email protected]
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

dashboard/channels/packages/dashboard/2.0.0/manifest.yaml Outdated
descriptor:
type: "kubernetes-dashboard"
description: |
This is a demo of the kubebuilder-declarative-pattern project and not a production ready installation.
Copy link
Contributor

Choose a reason for hiding this comment

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

ditto.

@atoato88
Copy link
Contributor

I've confirmed to pass a test.
/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Mar 16, 2020
@somtochiama somtochiama changed the title [WIP] Operator for the Kubernetes dashboard Operator for the Kubernetes dashboard Mar 17, 2020
@k8s-ci-robot k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Mar 17, 2020
@justinsb
Copy link
Contributor

Thanks @somtochiama , and thanks also to @atoato88 for working together to figure out the right way forwards.

Sounds like we have consensus, so...

/approve
/lgtm

We can always iterate on it as well - I think I want to figure out what to do about the config/ directories which are the bulk of the repetitive boilerplate.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: justinsb, SomtochiAma

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 17, 2020
@k8s-ci-robot k8s-ci-robot merged commit 9bba8d5 into kubernetes-sigs:master Mar 17, 2020
@somtochiama somtochiama deleted the dashboard-operator branch March 18, 2020 08:36
This was referenced Apr 6, 2020
Closed
Merged
@somtochiama somtochiama mentioned this pull request Aug 14, 2020
Closed
6 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@atoato88 atoato88 atoato88 left review comments

@justinsb justinsb Awaiting requested review from justinsb

@stealthybox stealthybox Awaiting requested review from stealthybox

@johnsonj johnsonj Awaiting requested review from johnsonj

Assignees

@justinsb justinsb

@atoato88 atoato88

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. 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/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@somtochiama @atoato88 @justinsb @k8s-ci-robot