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

Add apprepositories-webhook ClusterRole in kupeapps chart #2151

Merged
merged 3 commits into from
Nov 16, 2020

Conversation

antgamdia
Copy link
Contributor

Description of the change

After the discussion in PR #2150. A ClusterRole can be added by default during the Kubeapps installation via Helm.

Benefits

We henceforth will have an apprepositories-webhook ClusterRole and users just have to create a ServiceAccount and a RoleBinding to start using the webhook endpoint.

Possible drawbacks

I'm not aware right now of the consequences of changing the chart here. Perhaps there are some requirements. For now, I'm sending this PR as a draft.

Applicable issues

Additional information

N/A

@antgamdia antgamdia mentioned this pull request Nov 6, 2020
Copy link
Contributor

@andresmgot andresmgot left a comment

Choose a reason for hiding this comment

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

Just one naming suggestion.

apiVersion: rbac.authorization.k8s.io/v1
kind: ClusterRole
metadata:
name: "kubeapps:{{ .Release.Namespace }}:apprepositories-webhook"
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd call this kubeapps:{{ .Release.Namespace }}:apprepositories-refresh to follow the same naming than the others.

Copy link
Contributor Author

@antgamdia antgamdia Nov 9, 2020

Choose a reason for hiding this comment

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

Good idea. Since it involves changing the docs, I will also send a PR for that purpose.
EDIT: See #2153

@antgamdia antgamdia closed this Nov 10, 2020
@antgamdia antgamdia deleted the webhookClusterRole branch November 10, 2020 16:33
@antgamdia antgamdia restored the webhookClusterRole branch November 16, 2020 16:53
@antgamdia antgamdia reopened this Nov 16, 2020
@antgamdia antgamdia marked this pull request as ready for review November 16, 2020 16:54
@antgamdia
Copy link
Contributor Author

I've just noticed that this PR was closed but not merged (I initially configured it as a draft, so it couldn't be merged).
Merging now.

@antgamdia antgamdia merged commit b8524e8 into vmware-tanzu:master Nov 16, 2020
@antgamdia antgamdia deleted the webhookClusterRole branch November 16, 2020 16:56
metadata:
name: "kubeapps:{{ .Release.Namespace }}:apprepositories-refresh"
labels:{{ include "kubeapps.extraAppLabels" . | nindent 4 }}
app: {{ template "kubeapps.apprepository.fullname" . }}
Copy link
Contributor

Choose a reason for hiding this comment

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

We missed the indentation here - app: is a label, not a field on metadata - which caused CI to fail, but we currently have false positives so it makes a genuine failure easier to ignore. We should fix that :) Anyway, I've fixed it as part of #2162 .

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My bad! I didn't notice it! Thanks for pointing out and fix it!!!

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.

None yet

3 participants