search

dapr / php-sdk

Dapr SDK for PHP
Apache License 2.0
72 stars 15 forks source link

Replace custom CloudEvent implementation in favor of official CloudEvent SDK #129

Open hendrikheil opened 2 years ago

hendrikheil commented 2 years ago

Description

This PR aims to add support for the official CloudEvent PHP SDK while deprecating the use of the current CloudEvent implementation.

Issue reference

128

Checklist

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

codecov-commenter commented 2 years ago

Codecov Report

Merging #129 (34eadbd) into main (d17e37e) will decrease coverage by 0.03%. The diff coverage is 100.00%.

@@             Coverage Diff              @@
##               main     #129      +/-   ##
============================================
- Coverage     73.48%   73.44%   -0.04%     
- Complexity      562      565       +3     
============================================
  Files            72       72              
  Lines          1746     1766      +20     
============================================
+ Hits           1283     1297      +14     
- Misses          463      469       +6
Impacted Files Coverage Δ
src/lib/PubSub/CloudEvent.php 80.35% <ø> (ø)
src/lib/PubSub/Topic.php 82.60% <100.00%> (+2.60%) :arrow_up:
src/config.php 100.00% <0.00%> (ø)
src/lib/DaprClient.php 0.00% <0.00%> (ø)
src/lib/Client/BindingRequest.php 0.00% <0.00%> (ø)
src/lib/State/StateManagerOld.php 0.00% <0.00%> (ø)
src/lib/Client/HttpPubSubTrait.php 100.00% <0.00%> (ø)
src/lib/State/Internal/Transaction.php 100.00% <0.00%> (ø)
src/lib/State/TransactionalStateOld.php 0.00% <0.00%> (ø)
src/lib/Actors/ActorRuntime.php 98.79% <0.00%> (+0.01%) :arrow_up:
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d17e37e...34eadbd. Read the comment docs.

withinboredom commented 2 years ago

LGTM, I'll fix the integration tests unless you want to take a stab at it (looks like an issue with Laravel).

hendrikheil commented 2 years ago

Sure thing, I'll take a look

hendrikheil commented 2 years ago

Seemed to be an issue with the locked version of laravel/sail's 8.0 runtime. Simply getting a new lock file should do the trick :crossed_fingers:

yaron2 commented 2 years ago

@CiiDyR is this PR ready to be merged?

hendrikheil commented 2 years ago

Yep, should be good to go, same with #127

withinboredom commented 2 years ago

Thanks @CiiDyR! I have no idea what the DCO action is or why it is failing. I think I have to do something special, but I have no idea what nor do I currently have access to any chat channels to ask silly questions. I'll make a point to figure it out this week. But yeah, this can be merged as soon as I figure out this mysterious action that I didn't add to the repo means.

hendrikheil commented 2 years ago

I think I fixed it by amending the commits like described here: https://github.com/src-d/guide/blob/master/developer-community/fix-DCO.md

So should be ready for merging now, I guess at least...

withinboredom commented 2 years ago

Awesome! I'll get this merged this evening!

withinboredom commented 2 years ago

@CiiDyR, I've updated some bits, would you mind rebasing on the latest main? The composer.lock conflict is your earlier PR's changes.

hendrikheil commented 2 years ago

For some reason the rebase duplicated the commits, so in order to keep the history clean, I just made new commits. Not sure what tests are failing now, but it looks unrelated to the CloudEvent changes (at least to my untrained eyes) :)

hendrikheil commented 2 years ago

@withinboredom As far as I can tell the jobs fail because update-examples.php doesn't set the remote origin to the source repo. So composer looks for feature/cloudevent-replacement on dapr/php-sdk, which doesn't exist. As far as I can tell the best way to solve this would either be to add logic to add the repo to the composer repositories key. So I've just gone ahead and added something that should resolve the issues.

withinboredom commented 2 years ago

Thanks @CiiDyR! Sorry for the delay, my computer died a horrible death after 4 years of fantastic service. I've just got my new computer all set up and I'll be getting to this sometime in the next week.

Thanks again for your contribution here!

hendrikheil commented 2 years ago

Any updates on this PR? :)

withinboredom commented 2 years ago

Sorry @CiiDyR, I'll get to this soon!

withinboredom commented 2 years ago

In exactly 2 weeks, I'll be going on a sabbatical from regular work for 3 months. Until then, things are just too crazy to get this merged. Once I'm on my sabbatical, I plan to focus on this and a couple of other open-source projects.

I just wanted to give you a heads up @CiiDyR. I haven't forgotten about you or this PR.