Skip to content
This repository has been archived by the owner on Feb 22, 2022. It is now read-only.

[stable/kanister-operator] RBAC changes and kanister profile creation #6280

Merged
merged 8 commits into from Jul 15, 2018
Merged

[stable/kanister-operator] RBAC changes and kanister profile creation #6280

merged 8 commits into from Jul 15, 2018

Conversation

depohmel
Copy link
Contributor

Provides the option to create a profile CR along with the operator
the profile CR is created via profiles sub-chart

Kanister operator requires access to CRDs and Events.
Add this to the cluster-role.

Fixes kanisterio/kanister#82
Fixes kanisterio/kanister#90

[stable/kanister-operator] RBAC changes and default profile creation
c9e85da 
Provides the option to install a profile CR along with the operator
the profile CR is installed via a subchart

Kanister operator requires access to CRDs and Events.
Add this to the cluster-role.

Fixes kanisterio/kanister#82
fixes kanisterio/kanister#90
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 22, 2018
@depohmel
Copy link
Contributor Author

/assign @mgoodness
/assign @unguiculus

@depohmel depohmel changed the title [stable/kanister-operator] RBAC changes and default profile creation [stable/kanister-operator] RBAC changes and kanister profile creation Jun 22, 2018
unguiculus
unguiculus suggested changes Jun 25, 2018
View reviewed changes
stable/kanister-operator/requirements.yaml Outdated
@@ -0,0 +1,5 @@
dependencies:
Copy link
Member

Choose a reason for hiding this comment

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

This adds a dependency to an external chart. I'd like to discuss this with other maintainers first.

I noticed they copied this chart to their own repo, but the chart seems to be maintained here while you want to add a dependency to another chart from them. Seems kind of messy.

https://github.com/kanisterio/kanister/tree/master/helm/kanister-operator

Since you seem to contribute to kanister as well, could you clear up the picture?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi @unguiculus, thanks for your questions.
Here are only 2 maintainers me and @tdmanv. And we are both one of the main contributors to kanister.

Why are we adding a new dependency?
Kanister requires profiles for some of the operations. Profile is a combinations of Profile Custome Resource, Sercret[s] and ConfigMap[s]. To make thigns ease we desided to create the profile chart. This will allow customers to create profiles while deploying kanister-operator, apps or independatly.

Why this chart is stored in kansiter.charts repo?
Mainly because it's a helper chart and it doens't look like it belongs in k8s/charts
Also @mattfarina mentioned that community charts would support trusted repos, and kanister woudl liove to get into this list.

Why kanister-operator chart was copied into kasniter chart repo?
We had a cranch time and needed to release new verison urgently. Unfortunatly due amount of PRs, merge into community charts repo takes a few days :(
So we decided to host it our self and push an update to k8s/charts repo.
I well be syncing these 2 charts, untill kasniter repo will be added as trusted repo into community repo.

Hope this clears things up.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks for the explanations @depohmel - as we discussed on the call (not sure if you were there for that), the requirements in the stable repo should only point to charts in the stable repository (as per https://github.com/kubernetes/charts/blob/master/CONTRIBUTING.md#technical-requirements).

Can I suggest, instead of contributing profile as a separate chart in stable, embedding the profile chart in the charts/ directory of this chart. That way you are able to include this dependency without making it a top-level chart.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@prydonius, no, I missed that part :(
I'll add profile chart into /charts

stable/kanister-operator/Chart.yaml Outdated
@@ -1,13 +1,13 @@
apiVersion: v1
description: Kanister-operator Helm chart for Kubernetes
name: kanister-operator
version: 0.2.3
version: 0.2.4
Copy link
Member

Choose a reason for hiding this comment

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

I'd say this deserves a minor bump.

@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Jun 29, 2018
@depohmel
Copy link
Contributor Author

@prydonius, @unguiculus I've added the profile chart as a local one.

@prydonius
Copy link
Member

ping @unguiculus want to give this one another look?

@prydonius
Copy link
Member

/ok-to-test

@k8s-ci-robot k8s-ci-robot removed the needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. label Jul 3, 2018
@unguiculus
Copy link
Member

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jul 15, 2018
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: depohmel, unguiculus

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 Jul 15, 2018
@k8s-ci-robot k8s-ci-robot merged commit 598a8c7 into helm:master Jul 15, 2018
NicolasT added a commit to scality/charts that referenced this pull request Jul 15, 2018
@NicolasT
Merge remote-tracking branch 'kubernetes/master' into kibana-service-…
f2ef16a 
…labels

* kubernetes/master: (411 commits)
  [stable/efs-provisioner] Chart for efs-provisioner, a Kubernetes external-storage provisioner (helm#3233)
  [stable/filebeat] filebeat fixes (helm#6332)
  [stable/stolon] Add support for priorityClasses (helm#6607)
  [stable/external-dns] Add support for priorityClasses (helm#6606)
  [stable/minio] Add support for priorityClasses (helm#6604)
  [stable/cluster-autoscaler] Add support for priorityClasses (helm#6603)
  [stable/oauth2-proxy] Add support for priorityClasses (helm#6586)
  [stable/elasticsearch-exporter] add support for priorityClasses (helm#6584)
  [stable/traefik] adding support for traefik wildcard certificates (helm#6015)
  gcloud-sqlproxy: add tolerations and affinity to the Deployment (helm#6495)
  Review readme (helm#6399)
  [stable/mongodb-replicaset] Prometheus Metrics export (helm#6282)
  [stable/artifactory-ha] Typo fix: livessProbed->livenessProbed (helm#6462)
  [stable/artifactory] livessProbed->livenessProbed (helm#6461)
  [incubator/kube-spot-termination-notice-handler] Add Support for Tolerations (helm#5813)
  [stable/kanister-operator] RBAC changes and kanister profile creation (helm#6280)
  fix redis-ha NOTE.txt, stable/redis-ha don't create master-0 pod (helm#6131)
  [stable/concourse] add support for custom envvars for the web containers (helm#6441)
  upgrade to latest prometheus release 2.3.2 and alertmanager 0.15.1 (helm#6623)
  cert-manager: fast-forward to upstream 777ce6f4 (helm#6625)
  ...
Jnig pushed a commit to Jnig/charts that referenced this pull request Nov 13, 2018
@Jnig
[stable/kanister-operator] RBAC changes and kanister profile creation (…
44eac0c 
…helm#6280)

* [stable/kanister-operator] RBAC changes and default profile creation

Provides the option to install a profile CR along with the operator
the profile CR is installed via a subchart

Kanister operator requires access to CRDs and Events.
Add this to the cluster-role.

Fixes kanisterio/kanister#82
fixes kanisterio/kanister#90

* fixing repo url and version requirement

* Fix typo

* remove remote chart with local

* operator version to 0.10.0

* new lines EOF

* Update Chart.yaml

Signed-off-by: Jakob Niggel <info@jakobniggel.de>
gsemet pushed a commit to gsemet/charts that referenced this pull request Nov 13, 2018
@gsemet
[stable/kanister-operator] RBAC changes and kanister profile creation (…
aa219bc 
…helm#6280)

* [stable/kanister-operator] RBAC changes and default profile creation

Provides the option to install a profile CR along with the operator
the profile CR is installed via a subchart

Kanister operator requires access to CRDs and Events.
Add this to the cluster-role.

Fixes kanisterio/kanister#82
fixes kanisterio/kanister#90

* fixing repo url and version requirement

* Fix typo

* remove remote chart with local

* operator version to 0.10.0

* new lines EOF

* Update Chart.yaml
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@unguiculus unguiculus unguiculus approved these changes

@prydonius prydonius prydonius approved these changes

@mgoodness mgoodness Awaiting requested review from mgoodness

Assignees

@unguiculus unguiculus

@mgoodness mgoodness

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 Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@depohmel @prydonius @unguiculus @k8s-ci-robot @mgoodness