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

docs(argo-rollouts): add more description for notification secret creation #2922

Merged
merged 6 commits into from
Sep 16, 2024

Conversation

st-myk
Copy link
Contributor

@st-myk st-myk commented Sep 15, 2024

Checklist:

  • I have bumped the chart version according to versioning
  • I have updated the documentation according to documentation
  • I have updated the chart changelog with all the changes that come with this pull request according to changelog.
  • Any new values are backwards compatible and/or have sensible default.
  • I have signed off all my commits as required by DCO.
  • My build is green (troubleshooting builds).

Signed-off-by: st-myk <93274808+st-myk@users.noreply.github.com>
Signed-off-by: st-myk <93274808+st-myk@users.noreply.github.com>
@st-myk st-myk marked this pull request as ready for review September 15, 2024 12:21
Signed-off-by: st-myk <93274808+st-myk@users.noreply.github.com>
@@ -456,7 +456,8 @@ notifications:
create: true

secret:
# -- Whether to create notifications secret
# -- Whether to create notifications secret.
## If you want to manually create secret, do not forget to add proper label to it: "app.kubernetes.io/component: rollouts-controller".
Copy link
Collaborator

Choose a reason for hiding this comment

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

Thank you for your PR.
nits; The label in secret is app.kubernetes.io/component: {{ .Values.controller.component }}, so it's not always true that rollouts-controller is the right value. 🤔
Ref: https://github.com/argoproj/argo-helm/blob/argo-rollouts-2.37.6/charts/argo-rollouts/templates/controller/notifications-secret.yaml#L14

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Alright, I will proceed with referencing the {{ .Values.controller.component }} then

Copy link
Collaborator

Choose a reason for hiding this comment

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

Hi @st-myk
thank you your update. Please run ./scripts/helm-docs.sh to pass CI, as following below, the PR description.

I have updated the documentation according to documentation

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for your patience 🙂

Signed-off-by: st-myk <93274808+st-myk@users.noreply.github.com>
Signed-off-by: Mykola Stasiuk <93274808+st-myk@users.noreply.github.com>
@github-actions github-actions bot added size/S and removed size/XS labels Sep 16, 2024
Copy link
Collaborator

@yu-croco yu-croco 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 contribution. LGTM.

@mbevc1 mbevc1 merged commit 2e05c8b into argoproj:main Sep 16, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants