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

🐛 Helm chart: Allow writing logs to S3 easily #8577

Closed
wants to merge 6 commits into from
Closed

🐛 Helm chart: Allow writing logs to S3 easily #8577

wants to merge 6 commits into from

Conversation

vnourdin
Copy link
Contributor

@vnourdin vnourdin commented Dec 7, 2021

What

The current helm chart is aimed to be used with Minio (chart or external) but not for S3 location.

How

  • add values for S3 env variables
  • mutualize every logs-relative values in a logs section
  • adapt the env configmap
  • create or adapt helpers to manage every logging
  • remove direct access to minio values in worker and scheduler deployments

Also

  • also fixed two non-relative values in temporal and server deployments

🚨 User Impact 🚨

People switching from the old chart to this version must adapt their values, especially minio.* and externalMinio.* that moved to logs.minio.* and logs.externalMinio.*

@CLAassistant
Copy link

CLAassistant commented Dec 7, 2021

CLA assistant check
All committers have signed the CLA.

@github-actions github-actions bot added area/platform issues related to the platform kubernetes labels Dec 7, 2021
@vnourdin vnourdin changed the title bug Helm chart: Allow writing logs to S3 easily 🐛 Helm chart: Allow writing logs to S3 easily Dec 7, 2021
@marcosmarxm
Copy link
Member

thanks for the contribution @vnourdin, @davinchia will take a look soon :)

## @param logs.s3.bucket Bucket name where logs should be stored
## @param logs.s3.bucketRegion Region of the bucket (must be empty if using minio)
s3:
enabled: false
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added this value to be consistent with other logs destinations, bur it is not used yet.
Maybe we should remove it and tell in the doc that it's the "fallback" if no minio nor externalMinio is enabled ?

Copy link
Contributor

Choose a reason for hiding this comment

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

that's fine!

Copy link
Contributor

@davinchia davinchia 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 contribution. We are going to revamp this soon.

One question to make sure I understand.

Curious, did you test this locally for S3?

@vnourdin
Copy link
Contributor Author

Yes I have tested, and it is in use on our production instance using S3 !

@davinchia
Copy link
Contributor

@vnourdin looks good! can you resolve the conflicts so I can merge this in? Thanks again!

@vnourdin
Copy link
Contributor Author

ℹ️ I have been forced to delete then recreate my fork and even if my branch is still named the same, GitHub cannot reconnect this PR to it 😞 I opened a new PR which is up-to-date with master: #8736

@vnourdin vnourdin closed this Dec 13, 2021
@davinchia
Copy link
Contributor

all good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/platform issues related to the platform community kubernetes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants