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.

Sign up for GitHub

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

Jump to bottom

add deploymentAnnotations field in helm chart #864

Merged
merged 1 commit into from
Jun 30, 2022
Merged

add deploymentAnnotations field in helm chart #864

merged 1 commit into from
Jun 30, 2022

Conversation

surajnarwade
Copy link
Contributor

@surajnarwade surajnarwade commented Jun 22, 2022 •
edited
Loading

Description of the change

This will add commonAnnotation field in Helm chart

Benefits

This will allow users to set annotation on sealed -secrets controller deployment

Ref: #863

@surajnarwade surajnarwade force-pushed the deploymentannotation branch from 76b56be to 07f6880 Compare June 22, 2022 07:29
@surajnarwade surajnarwade mentioned this pull request Jun 22, 2022
Open
@surajnarwade surajnarwade temporarily deployed to vmware-image-builder June 22, 2022 07:31 Inactive
alvneiayu
alvneiayu requested changes Jun 23, 2022
View reviewed changes
Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

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

hi @surajnarwade

Thanks a lot for your PR. There are some comments from my side. Could you also include the key inside the README file and bump the version value inside the Chart.yaml to 2.3.0, please?

Álvaro

helm/sealed-secrets/values.yaml Outdated
@@ -363,3 +363,8 @@ metrics:
## @param metrics.dashboards.namespace Namespace where Grafana dashboard ConfigMap is deployed
##
namespace: ""

Copy link
Collaborator

@alvneiayu alvneiayu Jun 23, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Please, could you move this new section after the extraDeploy key.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

helm/sealed-secrets/templates/deployment.yaml Outdated
@@ -5,6 +5,9 @@ metadata:
name: {{ include "sealed-secrets.fullname" . }}
namespace: {{ include "sealed-secrets.namespace" . }}
labels: {{- include "sealed-secrets.labels" . | nindent 4 }}
{{- if .Values.deploymentAnnotations }}
annotations: {{- toYaml .Values.deploymentAnnotations | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Please use the key commonAnnotations instead of deploymentAnnotations

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

@alvneiayu alvneiayu added enhancement backlog Issues/PRs that will be included in the project roadmap labels Jun 23, 2022
@surajnarwade surajnarwade force-pushed the deploymentannotation branch from 07f6880 to 8fdc9a3 Compare June 23, 2022 08:00
@surajnarwade surajnarwade temporarily deployed to vmware-image-builder June 23, 2022 08:02 Inactive
alemorcuq
alemorcuq suggested changes Jun 23, 2022
View reviewed changes
Copy link
Collaborator

@alemorcuq alemorcuq left a comment

Choose a reason for hiding this comment

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

Thanks for the PR! Could you also add the common annotations to the rest of the deployed resources? Thanks!

helm/sealed-secrets/README.md Outdated
@@ -77,6 +77,7 @@ The command removes all the Kubernetes components associated with the chart and
| `fullnameOverride` | String to fully override sealed-secrets.fullname | `""` |
| `namespace` | Namespace where to deploy the Sealed Secrets controller | `""` |
| `extraDeploy` | Array of extra objects to deploy with the release | `[]` |
| `commonAnnotations`| common annotations | `[]` |
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
| `commonAnnotations`| common annotations | `[]` |
| `commonAnnotations`| Annotations to add to all deployed resources | `[]` |

Copy link
Contributor Author

Choose a reason for hiding this comment

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

added

helm/sealed-secrets/templates/deployment.yaml Outdated
@@ -5,6 +5,9 @@ metadata:
name: {{ include "sealed-secrets.fullname" . }}
namespace: {{ include "sealed-secrets.namespace" . }}
labels: {{- include "sealed-secrets.labels" . | nindent 4 }}
{{- if .Values.commonAnnotations }}
annotations: {{- toYaml .Values.commonAnnotations | nindent 8 }}
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
annotations: {{- toYaml .Values.commonAnnotations | nindent 8 }}
annotations: {{- toYaml .Values.commonAnnotations | 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.

updated

helm/sealed-secrets/values.yaml Outdated
@@ -15,6 +15,10 @@ namespace: ""
## @param extraDeploy [array] Array of extra objects to deploy with the release
##
extraDeploy: []
## @param commonAnnotations [object] common annotations
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
## @param commonAnnotations [object] common annotations
## @param commonAnnotations [object] Annotations to add to all deployed resources

@surajnarwade surajnarwade force-pushed the deploymentannotation branch from 8fdc9a3 to 677fcc2 Compare June 29, 2022 12:08
@surajnarwade surajnarwade temporarily deployed to vmware-image-builder June 29, 2022 12:11 Inactive
@alvneiayu alvneiayu self-requested a review June 30, 2022 08:54
alemorcuq
alemorcuq approved these changes Jun 30, 2022
View reviewed changes
Copy link
Collaborator

@alemorcuq alemorcuq left a comment

Choose a reason for hiding this comment

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

Thanks for the PR!

alvneiayu
alvneiayu approved these changes Jun 30, 2022
View reviewed changes
Copy link
Collaborator

@alvneiayu alvneiayu left a comment

Choose a reason for hiding this comment

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

LGTM

@alemorcuq alemorcuq merged commit 2282db8 into bitnami-labs:main Jun 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alvneiayu alvneiayu alvneiayu approved these changes

@alemorcuq alemorcuq alemorcuq approved these changes

@agarcia-oss agarcia-oss Awaiting requested review from agarcia-oss agarcia-oss is a code owner

Assignees
No one assigned
Labels
backlog Issues/PRs that will be included in the project roadmap enhancement
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@surajnarwade @alvneiayu @alemorcuq