-
-
Notifications
You must be signed in to change notification settings - Fork 644
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
Added recipe for the Prooph pack #320
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
This reverts commit 41dca24.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request does not pass validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
Let me know when you are ready for a review |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would suggest updating travis.yml in a separate PR.
@gquemener could you address my other comments?
.travis.yml
Outdated
@@ -25,4 +27,4 @@ install: | |||
- composer config extra.symfony.allow-contrib true | |||
|
|||
script: | |||
- composer req "${PACKAGES}" | |||
- composer req $(echo "$PACKAGES" | tr --delete '\n') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for not being clear. $PACKAGES
will never contain more that one package.
Ok, for handling the travis configuration in a separate PR. I'm taking care of your other comments, thanks. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
It's done @Nyholm, thanks for your comments 👍 |
👍 Yay! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you. Im happy with this PR. I left a small comment that you could fix if you want to.
I've tested this PR locally and it works great. To make sure it get merged, remove "WIP" from the title.
app.event_store.pdo_connection.mysql: | ||
class: \PDO | ||
arguments: | ||
- 'mysql:host=%env(MYSQL_HOST)%;dbname=%env(MYSQL_DBNAME)%' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You could use a environment variable like "MSQL_DSN" here instead.
Any updates here? |
This indeed seems to be the case.
Then it's not worth fixing at the moment. I suggest we go with what you have now and it needed/require we can always improve it later. |
It looks like everyone in this thread is happy at this point... Is there anything else blocking this now? Or maybe it's a release cycle that it's waiting on? (just asking) |
Hello 👋 Considering this summer announcement1 about dropping support on the prooph/service-bus package, I'd say this PR would need to be reworked before merged... Any thought @UFOMelkor @codeliner? Should it be simply closed? |
No, it's still supported for a while and used a lot.
…On Sat, Sep 29, 2018, 02:59 Gildas Quéméner ***@***.***> wrote:
Hello 👋
Considering this summer announcement1 about dropping support on the
prooph/service-bus package, I'd say this PR would need to be reworked
before merged...
Any thought @UFOMelkor <https://github.com/UFOMelkor> @codeliner
<https://github.com/codeliner>?
Should it be simply closed?
------------------------------
1 here
<https://www.sasaprolic.com/2018/08/the-future-of-prooph-components.html>
and here
<https://www.sasaprolic.com/2018/08/the-future-of-prooph-components.html>
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#320 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAYEvFObk_9MholJD4nwKEFmtfPpG1CRks5ufnF0gaJpZM4Sv8L1>
.
|
What @prolic said + the next service bus version could be a community driven project using symfony messenger but adding CQRS semantics on top of it. A future prooph pack could provide that. Anyway, service-bus is maintained until 31.12.2019. |
So IIRC this one is ready to merge? EDIT: |
If that's the case, then the |
Travis is failing because this PR contains recipes for many packages at the same time. I had provided a solution, but @Nyholm asked me to revert it. I am not sure what to do to fix the build then 🤔, would you have a suggestion @OskarStark ? I'm removing the WIP flag |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull request passes validation.
Nice 👍 EDIT: @gquemener sorry, I missed your question. @Nyholm can you elaborate on this commit? as @gquemener said that there is more than one package in |
The problem with travis could be, that you submitted many recipes in one PR and travis expects only one recipe per PR? |
Yes it is. The I managed to get this space delimited list by using |
Initial conversation is here