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

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from

Conversation

hendrikheil
Copy link
Contributor

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:

  • Tests pass
  • Created/updated tests
  • Extended the documentation

@hendrikheil hendrikheil force-pushed the feature/cloudevent-replacement branch from 6345314 to cea9fef Compare March 3, 2022 11:07
@codecov-commenter
Copy link

codecov-commenter commented Mar 3, 2022

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%) ⬆️
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%) ⬆️
... 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
Copy link
Contributor

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

@hendrikheil
Copy link
Contributor Author

Sure thing, I'll take a look

@hendrikheil
Copy link
Contributor Author

hendrikheil commented Mar 3, 2022

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 🤞

@yaron2
Copy link
Member

yaron2 commented Apr 7, 2022

@CiiDyR is this PR ready to be merged?

@hendrikheil
Copy link
Contributor Author

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

@withinboredom
Copy link
Contributor

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 hendrikheil force-pushed the feature/cloudevent-replacement branch 2 times, most recently from 8dd204b to 9473f3f Compare April 11, 2022 10:53
@hendrikheil
Copy link
Contributor Author

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
Copy link
Contributor

Awesome! I'll get this merged this evening!

@withinboredom
Copy link
Contributor

@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 hendrikheil force-pushed the feature/cloudevent-replacement branch from 446c25b to d17e37e Compare April 14, 2022 09:41
@hendrikheil hendrikheil reopened this Apr 14, 2022
@hendrikheil hendrikheil force-pushed the feature/cloudevent-replacement branch from ff09250 to 0ea3176 Compare April 14, 2022 09:46
@hendrikheil
Copy link
Contributor Author

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
Copy link
Contributor Author

hendrikheil commented Apr 21, 2022

@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
Copy link
Contributor

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
Copy link
Contributor Author

Any updates on this PR? :)

@withinboredom
Copy link
Contributor

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

@withinboredom
Copy link
Contributor

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.

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