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

Conversation

@jpds
Copy link
Collaborator

@jpds jpds commented Sep 10, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Sep 10, 2017
@k8s-ci-robot
Copy link
Contributor

Hi @jpds. Thanks for your PR.

I'm waiting for a kubernetes member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

I understand the commands that are listed here.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-ok-to-test Indicates a PR that requires an org member to verify it is safe to test. labels Sep 10, 2017
@viglesiasce
Copy link
Contributor

/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 Oct 16, 2017
Copy link
Member

@unguiculus unguiculus left a comment

Choose a reason for hiding this comment

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

Note that we've started namespacing templates (see #1785). You should apply this to your chart (can be done in another PR) in order for this chart to move to stable.

apiVersion: v1
metadata:
labels:
app: {{ template "fullname" . }}-config
Copy link
Member

Choose a reason for hiding this comment

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

app label should be {{ template "name" . }} on all resources.

component: {{ default "gitea" .Values.service.nameOverride }}
heritage: {{ .Release.Service }}
release: {{ .Release.Name }}
name: {{ template "fullname" . }}-config
Copy link
Member

Choose a reason for hiding this comment

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

All other resources are named {{ template "fullname" . }}. So should be the configmap.

labels:
app: {{ template "fullname" . }}-config
chart: "{{ .Chart.Name }}-{{ .Chart.Version }}"
component: {{ default "gitea" .Values.service.nameOverride }}
Copy link
Member

Choose a reason for hiding this comment

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

I don't see why a component label on the configmap would make sense. I'd suggest you remove it.

@unguiculus unguiculus self-assigned this Nov 3, 2017
@unguiculus
Copy link
Member

Marking this a stale. Please update within one week.

@viglesiasce
Copy link
Contributor

Closing as stale.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants