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

chore: prepare 0.12.2 release #1881

Merged
merged 1 commit into from
Aug 26, 2024
Merged

chore: prepare 0.12.2 release #1881

merged 1 commit into from
Aug 26, 2024

Conversation

djc
Copy link
Collaborator

@djc djc commented Aug 21, 2024

Prepare release for #1866, which fixes a regression from #1731.

Would like to get this merged before releasing:

@tottoto
Copy link
Collaborator

tottoto commented Aug 21, 2024

I think #1866 should be treated as a breaking change.

@djc
Copy link
Collaborator Author

djc commented Aug 21, 2024

I think #1866 should be treated as a breaking change.

What code do you think it breaks? It seems to strictly make more code work, in particular code that trivially worked pre-#1731 and for which there is no obvious/simple alternative. Some of my downstream code turns out to be affected, too, in that if call SomeServiceClient::new() and start calling methods on it, that just fails if I give it a https URL. How should I rewrite that code, in your opinion?

@tottoto
Copy link
Collaborator

tottoto commented Aug 21, 2024

Makes sense. I understand that it can be treated as not breaking change.

CHANGELOG.md Outdated Show resolved Hide resolved
@djc
Copy link
Collaborator Author

djc commented Aug 23, 2024

I've revised the changelog and added more things to it. I think this is ready. @LucioFranco can you get it released?

CHANGELOG.md Outdated Show resolved Hide resolved
Co-authored-by: tottoto <tottotodev@gmail.com>
@djc
Copy link
Collaborator Author

djc commented Aug 26, 2024

It does not matter if it is a side effect, it is whether it is intended or not.

So when you wrote #1731, did you consider (and intend) that SomeServiceClient::new() would no longer be able to handle https endpoints? I did not consider that while reviewing, and would have objected if I did. From that perspective, it is not entirely intentional.

From your comments it seems that you are picking up pieces of information and judging whether something is a bug or not based on your own preferences.

The 0.12.0 release is a major release that assumes the existence of breaking changes, so it is legitimate for it to include any breaking changes. Whether or not you like a change has no bearing upon whether or not it is a bug. It is a technical problem.

IMO it is reasonable to consider it a bug that SomeServiceClient::new() still compiled in 0.12.0 but failed to work with https endpoint URLs. While it may be technically allowed by semver, I think the effect of the changes in #1731 on that scenario are problematic for this reason. In general, if one (or two) maintainers think something is reasonable but most of the downstream users disagree, I'm inclined to think the users are right by default.

@djc djc added this pull request to the merge queue Aug 26, 2024
Merged via the queue into master with commit 82a856f Aug 26, 2024
16 checks passed
@djc djc deleted the prepare-0.12.2 branch August 26, 2024 12:44
@tottoto
Copy link
Collaborator

tottoto commented Aug 26, 2024

So when you wrote #1731, did you consider (and intend) that SomeServiceClient::new() would no longer be able to handle https endpoints?

It works with adding appropriate TLS configuration after that change. It is intended and the one of the main purposes of the changes.

@djc
Copy link
Collaborator Author

djc commented Aug 27, 2024

This was published: https://crates.io/crates/tonic/0.12.2.

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.

4 participants