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

[velero] Refactoring labels and annotations #281

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

nlamirault
Copy link

@nlamirault nlamirault commented Jun 30, 2021

Special notes for your reviewer:

  • support for custom annotations and custom labels for all resources
  • remove name label on 2 resources
  • split labels and selector labels (Helm recommendation)

Checklist

[Place an '[x]' (no spaces) in all applicable fields. Please remove unrelated fields.]

  • DCO signed
  • Chart Version bumped
  • Variables are documented in the values.yaml or README.md
  • Title of the PR starts with chart name (e.g. [velero])

Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Signed-off-by: Nicolas Lamirault <[email protected]>
Copy link
Collaborator

@jenting jenting left a comment

Choose a reason for hiding this comment

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

Thank you for your improvement to make the chart easier to read and configure.
But the CI is not passed, could you please take a look?

@jenting jenting added velero enhancement New feature or request labels Jul 5, 2021
@nlamirault
Copy link
Author

@jenting i don't understard CI error. Node not ready ?

@jenting
Copy link
Collaborator

jenting commented Jul 5, 2021

Okay, I'll look at it later.

@jenting
Copy link
Collaborator

jenting commented Jul 6, 2021

I push a commit to test it :)

charts/velero/templates/_helpers.tpl Outdated Show resolved Hide resolved
charts/velero/templates/_helpers.tpl Show resolved Hide resolved
charts/velero/templates/_helpers.tpl Show resolved Hide resolved
charts/velero/templates/_helpers.tpl Show resolved Hide resolved
Comment on lines 20 to 22
{{- with .Values.labels }}
{{- toYaml . | nindent 4 }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

With removing these lines, will this make the labels duplicated?

charts/velero/templates/schedule.yaml Outdated Show resolved Hide resolved
charts/velero/templates/schedule.yaml Outdated Show resolved Hide resolved
Comment on lines 9 to 11
{{- if .Values.customAnnotations }}
{{- toYaml .Values.customAnnotations | nindent 4 }}
{{- end }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Personally, I'd prefer.

    {{- With .Values.customAnnotations }}
    {{- toYaml . | nindent 4 }}
    {{- end }}

This makes the code simpler.

@jenting
Copy link
Collaborator

jenting commented Jul 19, 2021

CI was failed, could you please take a look?

Error:  templates/: template: velero/templates/schedule.yaml:11:20: executing "velero/templates/schedule.yaml" at <.Values.customAnnotations>: nil pointer evaluating interface {}.customAnnotations

@jenting jenting removed the request for review from carlisia August 31, 2021 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request velero
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants