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

Revert #5416, broke DI backward compatibility #5454

Merged
merged 1 commit into from
Dec 20, 2021

Conversation

Arkatufus
Copy link
Contributor

No description provided.

@Aaronontheweb Aaronontheweb added this to the 1.4.31 milestone Dec 20, 2021
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

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

LGTM

@Aaronontheweb Aaronontheweb enabled auto-merge (squash) December 20, 2021 22:47
@Aaronontheweb
Copy link
Member

I liked #5416 on balance, but due to the issues I identified in #5453 we can't let this hang out there in the open. Broke every Akka.DI plugin. Even though we're deprecating those we haven't communicated that to end users or set an explicit deadline. Can't break things by accident in a revision release like that.

@Aaronontheweb Aaronontheweb merged commit 362dc2f into akkadotnet:dev Dec 20, 2021
@Zetanova
Copy link
Contributor

What exactly break and why do we revert racy spec and bug fixes too?

If its only the IActorProducer interface we could change it back and use a better binary compatible version/pattern

@Aaronontheweb
Copy link
Member

Aaronontheweb commented Dec 20, 2021

We're reverting the entire PR because it was the most expedient way to fix the error. We can't tolerate a surprise breaking change like that affecting users' applications with no warning.

I would love to reintroduce your changes but they need to be done in smaller chunks; going forward I'm going to be more aggressive about rejecting PRs that get too big and touch too many things. Shouldn't be racy spec fixes and other bug fixes on that same PR that touches core API changes if those changes are unrelated. Could you please reintroduce those in smaller PRs?

As I mentioned here #5453 a few moments ago, I'm going to document our standards explicitly going forward. We have to.

@Aaronontheweb
Copy link
Member

For the record, this breaking change and the subsequent revert is my fault - I haven't helped us build an API change management system that is more resilient, I haven't documented the standards we need to make it easy to contribute to the project, I approved the PR. I'm going to bump that up to the top of my list for Jan 2022.

@Aaronontheweb
Copy link
Member

I'm sorry for making the process of contributing to Akka.NET more frustrating that necessary for you @Zetanova - that's my fault too. I'll put a dent in https://github.com/akkadotnet/akka.net/projects/7 and update CONTRIBUTORS.md and our pull request templates. I need to do that before a new full time engineer for Petabridge starts in January anyway.

@Zetanova
Copy link
Contributor

We expected that it will not binary compatible with Akka.DI, you wanted to test it and then it was merged.
I suspected that nobody is using Akka.DI

I will copy the fixes out, but not this week.

What we need is also a good system to mark things obsolete and to know when they can be deleted.
There still so many functions marked "obsolete" from version 1.0.0 (2014-2015)

@Aaronontheweb
Copy link
Member

We expected that it will not binary compatible with Akka.DI, you wanted to test it and then it was merged.
I suspected that nobody is using Akka.DI

You're absolutely right and that's on me.

@Aaronontheweb
Copy link
Member

What we need is also a good system to mark things obsolete and to know when they can be deleted.
There still so many functions marked "obsolete" from version 1.0.0 (2014-2015)

Agreed - we usually do this between minor versions, but again - I've not documented that.

@to11mtm
Copy link
Member

to11mtm commented Dec 21, 2021

We expected that it will not binary compatible with Akka.DI, you wanted to test it and then it was merged. I suspected that nobody is using Akka.DI

I will copy the fixes out, but not this week.

What we need is also a good system to mark things obsolete and to know when they can be deleted. There still so many functions marked "obsolete" from version 1.0.0 (2014-2015)

Ironically I -do- still use Akka.DI, although this hasn't yet impacted us yet as we are a little more conservative on non-critical package upgrades.

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