Remove dependency to dapr dapr

[tscolari]

Description

This is an attempt to remove the dependency between go-sdk and dapr/dapr.
It's a big limitation that an go application using the go-sdk also needs to bring in the dapr/dapr codebase, just for the use of protos. In more complex systems, this causes the dependency link between them to be prohibitive to updates, causing a chain reaction upon a version bump - it's also not nice to have such a big dependency in any project just to consume generated code.

The main goal of this change is to generate the used dapr/dapr protos in the go-sdk repo, and remove any code dependency with the dapr/dapr repo.

For that, a new make target make proto was added. It uses buf to fetch the dapr/dapr at compile time and generate the necessary protos.

There are "hand made" changes to the compiled protos in the dapr/dapr repo that were being used by the go-sdk.
To get around those, a new client/internal/crypto package was added, and instead of injecting methods to generated types it just uses generics to achieve the same results.

** Update 1:**

• I've added github actions to validate that the there are no differences in the generated proto files (that they didn't diverge).
• I've also added a simple lock mechanism to ensure that the proto version is only bumped intentionally. Ideally we would use buf's own version locking for this, but I think it requires registering dapr/dapr with on its registry.

Issue reference

#749

Please reference the issue this PR will close: #749

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

[codecov]

Codecov Report

Attention: Patch coverage is 4.42600% with 1382 lines in your changes missing coverage. Please review.

Project coverage is 23.53%. Comparing base (6c59092) to head (ad22dfc).
Report is 28 commits behind head on main.

Files with missing lines	Patch %	Lines
...rnal/proto/dapr/proto/runtime/v1/appcallback.pb.go	0.00%	716 Missing ⚠️
internal/proto/dapr/proto/common/v1/common.pb.go	0.00%	418 Missing ⚠️
...proto/dapr/proto/runtime/v1/appcallback_grpc.pb.go	0.00%	247 Missing ⚠️
client/internal/crypto/helpers.go	97.95%	1 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (6c59092) and HEAD (ad22dfc). Click for more details.

HEAD has 3 uploads less than BASE

Flag	BASE (6c59092)	HEAD (ad22dfc)
	4	1

Additional details and impacted files

@@             Coverage Diff             @@
##             main     #751       +/-   ##
===========================================
- Coverage   62.52%   23.53%   -39.00%     
===========================================
  Files          56       64        +8     
  Lines        4139    10977     +6838     
===========================================
- Hits         2588     2583        -5     
- Misses       1425     8257     +6832     
- Partials      126      137       +11     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[tscolari]

Ideally codecov should exclude the generated path ./internal/proto 🤔

[mikeee]

I take it buf is a direct replacement for protoc?

I don't have any major opinion either way but I'm not sure how generating them in the SDK is making it less prohibitive to updates with breaking changes to protos. Sure - the protos are intended as a source of truth but even as highlighted by the PR it would potentially involve changes and code sprawl that goes unmaintained in the future where buf is not maintaining the added package.

I'm actually for a clear contract with the runtime regarding exposed APIs so if a proposal can be drawn up with how this works out going forwards especially with the other SDKs considered I'd like to see this progressed. Is there a plan on making use of buf to the fullest extent replacing some existing tooling/actions? Perhaps generating all the protos in a separate repository - ignoring the code sprawl aspect might be something to consider also

[tscolari]

I take it buf is a direct replacement for protoc?
Yeah, the same can be achieved with protoc too I think, it's just more Ingenious.

I don't have any major opinion either way but I'm not sure how generating them in the SDK is making it less prohibitive to updates with breaking changes to protos. Sure - the protos are intended as a source of truth but even as highlighted by the PR it would potentially involve changes and code sprawl that goes unmaintained in the future where buf is not maintaining the added package.

I think ideally protos would be generated by CI, I can add this to the proposal here. It could work similar to the go.mod tidy checks, where if they are not up to date the check would fail. This would keep them in sync.
Technically protos should be compatible forward/backward, if we are using them correctly. I think the behavior would be the same in that aspect as importing them from the dapr repo - if we generate them on CI and compilation fails it would be similar as forgetting to bump dapr/dapr and fixing the breaking change 🤔

I'm actually for a clear contract with the runtime regarding exposed APIs so if a proposal can be drawn up with how this works out going forwards especially with the other SDKs considered I'd like to see this progressed. Is there a plan on making use of buf to the fullest extent replacing some existing tooling/actions? Perhaps generating all the protos in a separate repository - ignoring the code sprawl aspect might be something to consider also

I think this is probably the behavior of the other SDKs at the moment, since they can't consume the compiled go code from the dapr/dapr, For example the python sdk here - although I couldn't find how they build them.
Having a repo for the compiled protos to share between the gosdk and dapr could be an option, but it would still mean that the go-sdk is a bit special - even more with the cases that dapr/dapr has some custom code in the protos itself.

If it is less of a change I could try to remove buf in favor of protoc directly 🤔

[mikeee]

No, I'm actually pretty pro-using the best tool for the job and if buf is the new spanner of the day all is good.

Indeed most other SDKs have a script of some sort that pull a specified sha or tag and compile using protoc, this hasn't been ideal in most cases. Importing dapr/dapr has been the best way forward without having to manage protos but I just would like to see a path for migration clearly defined. E.g. can the helper functions be eliminated so we're left with a clear API surface?

[tscolari]

Cool! Yeah, I think buf does simplify things a lot.
I've added some manual version locking based on the dapr/dapr SHA, but that all can be replaced if dapr/dapr was in the buf registry. Technically then it could be used by all SDKs to generate their clients. But that needs probably to be a task for a maintainer.

We could get rid of the helper functions I think, but I let make a counter argument first:

I think the contract right now is still at the API surface already, what those helpers are currently doing is using generics to simplify setting/fetching some properties of the raw/generated Request/Response objects - they just avoid adding different code for 4 different types. The main problem with the dapr/dapr approach is that they are adding these methods directly tot he generated objects, which is not ideal (I've even added a safe-guard rm ./internal/proto/* in the make file target here to avoid this from happening in the future).

Initially I was worried that the modified types would have leaked to the sdk consumers, but they are only used internally, that's why it was easy to work around with the functions.

Just to get to my point, we can remove the functions, but we would still need code that makes it easier for example to set the Payload on 4 different types of objects. The helper methods is an approach using generics, I think other options would be to create some sort of decorator for them, but in the end they all would be doing the same thing.

In a nutshell what they are doing is mostly things like

x.Payload = this
// or

this = x.Payload

before sending or after fetching a response through the API (where x can be 4 different types).

[tscolari]

I've created an issue on dapr/dapr: dapr/dapr#8901, this would make things simpler here (and potentially for other SDKs too).

[mikeee]

I think this should wait until a general consensus is reached with other SDK maintainers before we go full steam ahead with this change.

[mikeee]

I'd like to see a git tag or hash passed here ideally