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

feat(deps)!: sentry update to 24.7.1 #1343

Conversation

TartanLeGrand
Copy link
Contributor

@TartanLeGrand TartanLeGrand commented Jun 29, 2024

This PR introduces updates to our Sentry Kubernetes configuration to align with the new features in Sentry 24.7.1. The latest version of Sentry autonomously handles Kafka migrations and topic management, prompting some significant changes in our setup.

Changes:

  • 🗑 Removed Kafka Topics Provisioning: Since Sentry 24.6.0 manages Kafka topics internally, I have removed the Kafka topics previously defined in the values.yaml.
  • 🔄 Moved and Optimized Jobs: Shifted several jobs to new locations and removed some redundant jobs. Considering the potential for further optimization, possibly through the use of init containers in future updates.
  • 📂 Reorganized Chart Directory: Moved charts into a dedicated charts folder to streamline chart linting and testing processes.

Issues Addressed:

Pending:

  • 🐛 There's an outstanding issue with the onboarding page that seems to stem directly from Sentry. I'm in the process of investigating this bug.

Next Steps:

  • 🧹 To maintain clarity and ease of review, I plan to split these changes into smaller, focused PRs before finalizing the merge.

Request for Feedback:

  • 🔄 I welcome any feedback on the job optimizations and the proposed use of init containers for future improvements.

Looking forward to your thoughts and suggestions!

@TartanLeGrand TartanLeGrand force-pushed the feat/sentry-update branch 9 times, most recently from bb60739 to 86e9ef5 Compare June 30, 2024 07:45
@TartanLeGrand TartanLeGrand force-pushed the feat/sentry-update branch 2 times, most recently from bc4cfdb to 821bf08 Compare July 2, 2024 20:52
@TartanLeGrand TartanLeGrand force-pushed the feat/sentry-update branch 3 times, most recently from 1d33577 to 3e8265e Compare July 31, 2024 11:32
@TartanLeGrand TartanLeGrand marked this pull request as ready for review July 31, 2024 13:04
@TartanLeGrand
Copy link
Contributor Author

LGTM, but IDK why the test does not work ...

@Mokto
Copy link
Contributor

Mokto commented Jul 31, 2024

I personally think it was better before when we could configure the topics.

Right now with the automated we enforce partition count = 1 everywhere?

@TartanLeGrand
Copy link
Contributor Author

I personally think it was better before when we could configure the topics.

Right now with the automated we enforce partition count = 1 everywhere?

I thinks for the topics we need to make an PR on the official repo 😄

I have revert my perf optimization and I let 3 replicas for the resources 👍

@TartanLeGrand
Copy link
Contributor Author

Let's make an inherited BC. I go refactor the PR for fix the Nginx before update. 😄

@TartanLeGrand TartanLeGrand changed the title feat(deps): sentry update to 24.6.0 feat(deps)!: sentry update to 24.6.0 Aug 1, 2024
@TartanLeGrand
Copy link
Contributor Author

@Mokto add this to the same major release.

@Mokto
Copy link
Contributor

Mokto commented Aug 16, 2024

I'm still against merging such a PR. We should be able to set the partition counts.

@adonskoy
Copy link
Contributor

I suggest then to update only the version. 24.8.0 has already been released, and the chart still uses the May release..

@Mokto
Copy link
Contributor

Mokto commented Aug 16, 2024

yeah that makes sense.

@TartanLeGrand TartanLeGrand changed the title feat(deps)!: sentry update to 24.6.0 feat(deps)!: sentry update to 24.8.0 Aug 18, 2024
@TartanLeGrand TartanLeGrand force-pushed the feat/sentry-update branch 2 times, most recently from 8e00965 to c191717 Compare August 19, 2024 08:26
@TartanLeGrand
Copy link
Contributor Author

TartanLeGrand commented Aug 21, 2024

24.8.0 doesn't work because we got an error of ClickHouse. But 24.7.1 does, I intend to migrate to 24.7.1 and fix 24.8.0 with the ClickHouse redesign. 🤔

@TartanLeGrand TartanLeGrand changed the title feat(deps)!: sentry update to 24.8.0 feat(deps)!: sentry update to 24.7.1 Aug 21, 2024
@Mokto
Copy link
Contributor

Mokto commented Aug 21, 2024

@TartanLeGrand just so that we're clear, you still intend to remove your changes related to --create-kafka-topics ?

@TartanLeGrand
Copy link
Contributor Author

TartanLeGrand commented Aug 21, 2024

Hello @Mokto, yes am sure it's related to getsentry/self-hosted#3121.

Let to sentry the responsibility of that.

@Mokto
Copy link
Contributor

Mokto commented Aug 21, 2024

As I said before, I don't want our users to lose the ability to set custom partition counts.

The self-hosted project is different as everything runs on 1 machine.

@Mokto
Copy link
Contributor

Mokto commented Aug 21, 2024

Thanks! I think it's ready to be merged?

@fseguin-pass
Copy link

FYI there is a security fix in 24.7.1 cf GHSA-fm88-hc3v-3www

@TartanLeGrand
Copy link
Contributor Author

Thanks! I think it's ready to be merged?

Nop, fail install with this version

@TartanLeGrand
Copy link
Contributor Author

@Mokto

I understand your concerns about managing Kafka partitions, but I believe we should let Sentry handle the topic creation as outlined in their Kafka schemas repository.

In my opinion, it's the software's responsibility not the infrastructure's to manage kafka topics provisioning. In a modern software approach, the infrastructure should provide the necessary resources for the software to function properly, while the software takes care of provisioning what it needs to operate efficiently.

@Mokto
Copy link
Contributor

Mokto commented Aug 23, 2024

Agreed but right now the software doesn't support it.

@Mokto
Copy link
Contributor

Mokto commented Sep 14, 2024

It's now using 24.7.1.

@Mokto Mokto closed this Sep 14, 2024
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.

6 participants