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

Remove deprecated ZipkinSender #43052

Conversation

wickdynex
Copy link
Contributor

Description

This PR have removed three SenderConfiguration also its Sender: UrlConnectionSenderConfiguration, RestTemplateSenderConfiguration and WebClientSenderConfiguration in file ZipkinConfigurations. Remove the related test cases about these SenderConfiguration class in file ZipkinConfigurationsSenderConfigurationTests.


Related Issue

Fixed #42589, #43047, #43048


Type of Change

  • Remove feature
  • Remove related tests
  • Add new test cases
  • Documentation update

Checklist

  • I have reviewed the code for any potential issues or improvements.
  • I have run tests locally and they are passing.
  • I have followed the coding style and conventions of the project.

Screenshots

image

Additional Notes

I does remove the other Senders and its SenderConfiguration class, and also remove the test class related to these deprecated🥰. But do I need refactor ZipkinConfiguration structure, and write some test case to integrate test this modified class.🤨 Also update the documents about this related Sender🤔. Please give me some advice if this PR can't be merged🥹, and I will try my best to fix it up. Looking forward to your reply❤️~

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Nov 6, 2024
@wickdynex wickdynex force-pushed the remove-deprecated-zipkin-sender branch from 71369e4 to 5e74bdf Compare November 6, 2024 17:15
@wilkinsona
Copy link
Member

Thanks for the PR, @wickdynex. Unfortunately, this is changing too many different things for a single PR. Please just focus on the changes for #42589 as already described by @mhalbritter:

In the scope of this issue, you should just reorder the configurations, like you said in #42589 (comment). And I guess some tests needs to be adapted.

@wickdynex
Copy link
Contributor Author

wickdynex commented Nov 7, 2024

Thanks for the PR, @wickdynex. Unfortunately, this is changing too many different things for a single PR. Please just focus on the changes for #42589 as already described by @mhalbritter:

In the scope of this issue, you should just reorder the configurations, like you said in #42589 (comment). And I guess some tests needs to be adapted.

Alright, maybe I should split this PR into 3 pieces for 3 issues🤔? I feel sorry to deal many issues in a single PR, it makes code review difficult🥺. emmm, and how to deal with this PR, close or continually commit on this branch? do u have any idea?🥰 And also assign the issue #43047, #43048 to me? If I can fix it🥳.

@wilkinsona
Copy link
Member

how to deal with this PR, close or continually commit on this branch?

You can either:

And also assign the issue #43047, #43048 to me? If I can fix it🥳

One thing at a time please. Let's get a PR for #42589 into a state where it's ready to be merged before considering the other two issues.

@snicoll
Copy link
Member

snicoll commented Nov 10, 2024

I can see you created a new PR. closing this one

@snicoll snicoll closed this Nov 10, 2024
@snicoll snicoll added status: duplicate A duplicate of another issue and removed status: waiting-for-triage An issue we've not yet triaged labels Nov 10, 2024
@wickdynex
Copy link
Contributor Author

I can see you created a new PR. closing this one

Yeah, thanks for your help🥳.

@wilkinsona wilkinsona added status: superseded An issue that has been superseded by another and removed status: duplicate A duplicate of another issue labels Nov 10, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Make ZipkinHttpClientSender the default BytesMessageSender
4 participants