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

OPSEXP-2877 Add alfresco-audit-storage chart #393

Merged
merged 12 commits into from
Oct 15, 2024
Merged

Conversation

pmacius
Copy link
Contributor

@pmacius pmacius commented Oct 11, 2024

@pmacius pmacius requested review from gionn and alxgomz October 14, 2024 12:31
Comment on lines 15 to 18
checksum/config-es: {{ include (print $.Template.BasePath "/configmap-es.yaml") $ | sha256sum }}
checksum/config-mq: {{ include (print $.Template.BasePath "/configmap-mq.yaml") $ | sha256sum }}
checksum/secret-es: {{ include (print $.Template.BasePath "/secret-es.yaml") $ | sha256sum }}
checksum/secret-mq: {{ include (print $.Template.BasePath "/secret-mq.yaml") $ | sha256sum }}
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not use that as it do not fit with the approach we use widely to offer usage of exisitingConfigmap (and has other drawbacks). We've been removing it from other charts (although some are probably still around). I think we should have a consistent recommendation of using proper tools like reloader

Copy link
Member

Choose a reason for hiding this comment

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

I think we never took a final decision on this, for me they are still worth to have because sometimes they help the end users without requiring any external tooling, so dropping them mean making life harder every time for end users.

Copy link
Contributor

Choose a reason for hiding this comment

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

To me keeping them means setting expectations we can't fulfil or even trigger unwanted service restarts (including in case where someone simply doesn't pay attention to that "feature"). While if someone needs to have that feature and use a dedicated tool for that he'll have full control and knowledge of how this works and it'll work as HE configured it.
Finally if someone really wants to implement that type of service reload., he should be able to do so using annotations.

Copy link
Member

Choose a reason for hiding this comment

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

service restarts should not really be an issue on a kubernetes environment, plus this approach is also documented on helm docs https://helm.sh/docs/howto/charts_tips_and_tricks/#automatically-roll-deployments

however not a big deal for me if you prefer to get rid of it

charts/alfresco-audit-storage/values.yaml Outdated Show resolved Hide resolved
github-advanced-security[bot]

This comment was marked as outdated.

@pmacius pmacius requested review from gionn and alxgomz October 14, 2024 15:10
@pmacius pmacius marked this pull request as ready for review October 14, 2024 15:10
@pmacius pmacius requested a review from gionn October 15, 2024 09:25
@pmacius pmacius merged commit db10b67 into main Oct 15, 2024
9 checks passed
@pmacius pmacius deleted the OPSEXP-2877-add-chart branch October 15, 2024 10:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants