Skip to content

Conversation

@mingmcb
Copy link
Contributor

@mingmcb mingmcb commented Sep 26, 2023

support customized labels

Master Issue: #156

Motivation

to add allow user add customized labels

Modifications

Update the deployment template for new labels

Verifying this change

  • Make sure that the change passes the CI checks.

This change is a trivial rework / code cleanup without any test coverage.

Documentation

Check the box below.

Need to update docs?

  • doc-required
  • no-need-doc
    this change is self-explain
  • doc

@mingmcb mingmcb requested a review from a team as a code owner September 26, 2023 23:56
@github-actions github-actions bot added the no-need-doc This pr does not need any document label Sep 26, 2023
@ericsyh
Copy link
Contributor

ericsyh commented Sep 27, 2023

@mingmcb thx for your contribution! just few suggestions for this PR:

  1. Can we rename to use labels and podLabels? which I think will be more consistent with some existing fields like the podAnnotations.
  2. We should also add these two fields in the https://github.com/streamnative/pulsar-resources-operator/blob/main/charts/pulsar-resources-operator/values.yaml then other users can notice them.

@ericsyh ericsyh added this to the 0.4.4 milestone Sep 27, 2023
ericsyh
ericsyh previously approved these changes Sep 28, 2023
Copy link
Contributor

@ericsyh ericsyh left a comment

Choose a reason for hiding this comment

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

LGTM

namespace: {{ include "pulsar-resources-operator.namespace" .}}
labels:
{{- include "pulsar-resources-operator.labels" . | nindent 4 }}
{{- toYaml .Values.labels" | nindent 4 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- toYaml .Values.labels" | nindent 4 }}
{{- toYaml .Values.labels | nindent 4 }}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good catch! please review

{{- toYaml .Values.podAnnotations | nindent 8 }}
labels:
{{- include "pulsar-resources-operator.selectorLabels" . | nindent 8 }}
{{- toYaml .Values.podLabels" | nindent 8 }}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
{{- toYaml .Values.podLabels" | nindent 8 }}
{{- toYaml .Values.podLabels | nindent 8 }}

Copy link
Contributor

@ericsyh ericsyh left a comment

Choose a reason for hiding this comment

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

Should remove unexpected " in the templates.

Copy link
Contributor

@ericsyh ericsyh left a comment

Choose a reason for hiding this comment

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

left a comment.

# follow Semantic Versioning. They should reflect the version the application is using.
# It is recommended to use it with quotes.
appVersion: "v0.4.3"
appVersion: "v0.4.4"
Copy link
Contributor

Choose a reason for hiding this comment

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

@mingmcb appVersion means the operator image version here.
So for your case, should not change the appVersion here cause your change is only about the chart template and won't need a new operator image.

Copy link
Contributor

@ericsyh ericsyh left a comment

Choose a reason for hiding this comment

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

LGTM

@ericsyh ericsyh merged commit 2e56e27 into streamnative:main Sep 29, 2023
@mingmcb mingmcb deleted the patch-1 branch September 29, 2023 11:58
ericsyh pushed a commit that referenced this pull request Oct 31, 2023
* support customized labels

* clean up the change

* Update deployment.yaml

* updated per review

(cherry picked from commit 2e56e27)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

no-need-doc This pr does not need any document

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants