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

Introduce patches with external kafka #3521

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

aldy505
Copy link
Collaborator

@aldy505 aldy505 commented Jan 11, 2025

Warning

This is a work in progress. Haven't test it myself. Blocked by Relay configuration. Talked to David Herberth, seems like we can't use environment variables for defining Kafka config for Relay at all.

Continuation of getsentry/sentry-docs#11847

Legal Boilerplate

Look, I get it. The entity doing business as "Sentry" was incorporated in the State of Delaware in 2015 as Functional Software, Inc. and is gonna need some rights from me in order to utilize my contributions in this here PR. So here's the deal: I retain all rights, title and interest in and to my contributions, and by keeping this boilerplate intact I confirm that Sentry can use, modify, copy, and redistribute my contributions, under Sentry's choice of terms.

@aldy505 aldy505 marked this pull request as draft January 11, 2025 13:22
@aldy505
Copy link
Collaborator Author

aldy505 commented Jan 11, 2025

@BYK Do you want to take a look?

Copy link
Member

@BYK BYK left a comment

Choose a reason for hiding this comment

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

Good start but I'd extract the patch files from the shell script for readability and maintainability.

You may rename the top-level patches folder you created to optional-modifications and then have an actual patches folder under that which has the patch files namespaces by the script name like external-kafka/sentry.conf.py.patch etc.

EOF
)
if [[ -f "docker-compose.override.yml" ]]; then
echo "🚨 docker-compose.override.yml already exists. You will need to modify it manually:"
Copy link
Member

Choose a reason for hiding this comment

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

Maybe try to be more helpful by sharing what needs to be changed in the file? Like "you need to remove the kafka service" or something among those lines.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Although I print the docker-compose.override.yml content, yeah, this might be helpful too.

@aldy505
Copy link
Collaborator Author

aldy505 commented Jan 12, 2025

You may rename the top-level patches folder you created to optional-modifications and then have an actual patches folder under that which has the patch files namespaces by the script name like external-kafka/sentry.conf.py.patch etc.

I don't want to have it as a patch file since I want the modified file to be specific. On this PR, I modify sentry.conf.py directly, in which for patch file, I'd modify sentry.conf.example.py instead, since there are no guarantees that sentry.conf.py exists.

@aldy505 aldy505 marked this pull request as ready for review January 12, 2025 01:08
@aldy505 aldy505 requested review from hubertdeng123 and BYK January 12, 2025 01:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: No status
Development

Successfully merging this pull request may close these issues.

2 participants