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

Bug/476 multiple methods per interface with JSON serialization doesn´t work #1343

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

Conversation

paule96
Copy link

@paule96 paule96 commented Aug 28, 2024

Description

Now the Actor manager provides the MethodName for the serialization ActorMessageSerializersManager.
This makes it now possible for the JSON serialization to support Actor interfaces with more than one method defined in it.

As a side effect, this PR solves #1342 too.
Changes made in the dev container:

  • use docker-in-docker instead of docker-from-docker
    • now the devcontainer is isolated from the host and is not affected by the host configuration. But it also means accessing the dapr instance of the devcontainer is only accessible from inside the container.
  • install all dotnet sdks that the tests requires. (dotnet 6 is still the default, maybe we should change that too)
  • add the C# DevKit as an additional extension. This can be a breaking change for people that don't have a visual studio license and want to run the dev container on their local host. Users that run the dev container on GitHub get a license automatically injected.
  • Unblocks the localinit.sh script before running it

Issue reference

It's part of the issue #476 starting with this comment.

It fixes also #1342

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

  • Code compiles correctly
  • Created/updated tests
  • Extended the documentation
    • partly, I added at the interesting locations in the code comments.
    • This change shouldn't affect the current documented actor model, it just fixes a broken feature

cc @onionhammer as original contributor :)

@paule96
Copy link
Author

paule96 commented Aug 28, 2024

@onionhammer it would be nice if you could try out these changes. For me, it seems to not be possible yet. I will look the next days into #1342 to solve the debugging/contribution story for others.

I did found out what is wrong with the devcontainer setup :) Will fix both here. Of course the blind implementation was broken. Will be fixed in the next hour.

@paule96 paule96 force-pushed the bug/476_multiple_methods_per_interface_with_json_serialisation_doesntwork branch 2 times, most recently from f847e87 to a13100b Compare August 28, 2024 15:24
@paule96 paule96 marked this pull request as ready for review August 28, 2024 15:24
@paule96 paule96 requested review from a team as code owners August 28, 2024 15:24
@paule96
Copy link
Author

paule96 commented Aug 28, 2024

@onionhammer This is now ready to review :) That was a fun research. I tried to don't break existing code paths as much as I could.

Copy link
Contributor

@onionhammer onionhammer left a comment

Choose a reason for hiding this comment

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

This looks good to me, although I have no authority here :)

cc @halspang @shivamkm07

@WhitWaldo
Copy link
Contributor

WhitWaldo commented Sep 2, 2024

install all dotnet sdks that the tests requires. (dotnet 6 is still the default, maybe we should change that too)

.NET 7 fell out of support May 14 of this year and .NET 8 will do the same on November 10, 2024 with .NET 9 being introduced in the same time frame. That should be right around the 1.15 release, so I think there will be a good case to be made to drop .NET 6 and .NET 7 explicit support, target .NET 8 instead and include .NET 9 in the test suite for 1.16.

paule96 and others added 8 commits September 3, 2024 08:48
Signed-off-by: paule96 <paul-jeschke@outlook.com>
Signed-off-by: paule96 <paul-jeschke@outlook.com>
…terface

Signed-off-by: paule96 <paul-jeschke@outlook.com>
Now the devcontainer uses docker in docker, so you can reach the dapr setup after you did run dapr init. This will then only affect the dev container, without compromising the host of the devcontainer

Signed-off-by: paule96 <paul-jeschke@outlook.com>
Signed-off-by: paule96 <paul-jeschke@outlook.com>
Signed-off-by: paule96 <paul-jeschke@outlook.com>
Signed-off-by: paule96 <paul-jeschke@outlook.com>
Signed-off-by: paule96 <paul-jeschke@outlook.com>
@paule96 paule96 force-pushed the bug/476_multiple_methods_per_interface_with_json_serialisation_doesntwork branch from adb8471 to 7be9077 Compare September 3, 2024 08:48
@paule96
Copy link
Author

paule96 commented Sep 3, 2024

@WhitWaldo if I change the default version to dotnet 9.0 the new security vulnerability scanning of the dotnet CLI turns on implicitly and produces, of course, a lot of errors.

I guess the upgrading of the default SDK needs to be delayed.

maybe a separate issue is needed to clean up the solution of, the old tests, old implementation, and what else is connected to it. I guess the documentation needs to be updated too.
In that issue, the devcontainer shouldn't be forgotten.

So for now I would try to just update the default to dotnet 8.0 (support until November 10, 2026, it's LTS), and postpone the change to dotnet 9.0 to a later point in time.

@WhitWaldo
Copy link
Contributor

That's right - the plan is to put off changing the targeted releases until after the release of Dapr 1.15. There, we'll target .NET 6 and .NET 8 still, so no changes needed in the short term. But we'll put something prominent announcing EOL in the release notes so it's clear that in 1.16 we'll drop support for .NET 6 in favor of .NET 8 (and language version C# 12). An open question on that thread is whether we should go on targeting .NET 9 as well (though I'm also inclined to lean yes since there are frequently so many performance improvements in each).

You're right, that'll require some significant doc updates in addition to the other projects, but I've got a draft PR that'll likely handle the latter as I'm gradually adding nullability annotations throughout the .NET SDK and there are several other PRs in place that'll shift up some of the project layouts as well that'll help.

Things to tackle after the next release!

@paule96
Copy link
Author

paule96 commented Sep 12, 2024

@WhitWaldo do you know anybody I could ping to get a review? (maybe yourself)

@WhitWaldo
Copy link
Contributor

@paule96 I'm not a maintainer, so my check isn't worth a whole lot. I suspect this will get more attention as we edge closer to the 1.15 release and there's more of a general code freeze. If it hasn't been looked at closer to that timeline, I'll give it a look myself (for whatever it might be worth).

@yaron2
Copy link
Member

yaron2 commented Sep 26, 2024

cc @halspang @philliphoff

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