Skip to content

Amqp interop based version. #154

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

Merged
merged 20 commits into from
Nov 8, 2017

Conversation

makasim
Copy link
Contributor

@makasim makasim commented Oct 26, 2017

ref #153

This is far from the final version, the aim of this PR is to show differences in approaches.

@vyuldashev
Copy link
Owner

I like the approach, I guess we can use it in v6.0. Does enqueue support SSL connections?

@makasim
Copy link
Contributor Author

makasim commented Oct 27, 2017

Does enqueue support SSL connections?

Not yet, I am going to work on it before continue with this PR

@makasim
Copy link
Contributor Author

makasim commented Oct 27, 2017

now Enqueue supports secure connections: php-enqueue/enqueue-dev#246

@vyuldashev
Copy link
Owner

Nice, so I wait for your pull request to be complete.

@makasim
Copy link
Contributor Author

makasim commented Oct 30, 2017

The code was tested manually in a sandbox app. More or less working.

Can I use phpunit built-in mocker in the tests?

@vyuldashev
Copy link
Owner

@makasim It's better to use real rabbitmq instance in tests.

@makasim
Copy link
Contributor Author

makasim commented Oct 31, 2017

@makasim It's better to use real rabbitmq instance in tests.

I don't understand. Would you like to have functional tests that interact with a real RabbitMQ broker?

@vyuldashev
Copy link
Owner

@makasim It would be awesome.

@makasim
Copy link
Contributor Author

makasim commented Oct 31, 2017

I am fine with writing them though I don't think it worth it. Enqueue transports have a lot of functional tests.

@makasim makasim changed the title [WIP] amqp interop based version. Amqp interop based version. Nov 1, 2017
@makasim
Copy link
Contributor Author

makasim commented Nov 1, 2017

@vyuldashev ready for review. There is one failing test (ssl connection). I am going to fix it tomorrow beside that everything else is ready.

@makasim
Copy link
Contributor Author

makasim commented Nov 2, 2017

@vyuldashev All is green and ready for review

@makasim
Copy link
Contributor Author

makasim commented Nov 6, 2017

@vyuldashev When will you have time to review?

@vyuldashev
Copy link
Owner

@makasim Is it already ready? I see tests are failing. Did you test this version on a Laravel app?

@makasim
Copy link
Contributor Author

makasim commented Nov 8, 2017

@vyuldashev yes it is ready, tests failed because of the latest fix. I'll fix them asap.

I tested several flows here php-enqueue/enqueue-sandbox#9

@makasim
Copy link
Contributor Author

makasim commented Nov 8, 2017

I'll be able to provide support in case of issues after merging it. I'll keep an eye on issues and comment or you can ping me

@vyuldashev vyuldashev merged commit 3dd712f into vyuldashev:master Nov 8, 2017
@makasim makasim deleted the amqp-interop branch November 8, 2017 11:16
@vyuldashev
Copy link
Owner

@makasim I merged your PR, thanks. But before releasing I want to make some changes.

@vyuldashev
Copy link
Owner

@makasim This version is available as dev-master / 6.0.x-dev for now.

@vrubiella
Copy link

@makasim I'm sorry for breaking the tests, I see you fixed now. 👍

@makasim
Copy link
Contributor Author

makasim commented Nov 8, 2017

@vrubiella np

@makasim
Copy link
Contributor Author

makasim commented Nov 16, 2017

@vyuldashev is there any ETA for rolling 6.0 release?

@vyuldashev
Copy link
Owner

@makasim Next week I will have a look and make a release.

@makasim
Copy link
Contributor Author

makasim commented Dec 7, 2017

@vyuldashev are there any news on the release? thanks

@vyuldashev
Copy link
Owner

@makasim sorry was very busy. Let’s release it tonight. Did you test 6.0-dev? Does it work as expected with new configuration and amqp-interop?

@makasim
Copy link
Contributor Author

makasim commented Dec 7, 2017

works for us.

@vyuldashev
Copy link
Owner

Released.

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