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

Add RabbitMQ as recommended system-requirement #16885

Merged
merged 4 commits into from
Apr 22, 2024

Conversation

skoch98
Copy link
Contributor

@skoch98 skoch98 commented Apr 1, 2024

As discussed with Bernhard Rusch at the Inspire, I am now adding RabbitMQ as a recommended system requirement in the documentation. I tried to present RabbitMQ's arguments as convincingly as possible... 😄

Copy link

github-actions bot commented Apr 1, 2024

Review Checklist

  • Target branch (11.2 for bug fixes, others 11.x)
  • Tests (if it's testable code, there should be a test for it - get help)
  • Docs (every functionality needs to be documented, see here)
  • Migration incl. install.sql (e.g. if the database schema changes, ...)
  • Upgrade notes (deprecations, important information, migration hints, ...)
  • Label
  • Milestone

@brusch
Copy link
Member

brusch commented Apr 2, 2024

LGTM, I think we should also include an example of how to configure RabbitMQ, that would make it more convenient for users 😊

@skoch-twocream
Copy link
Contributor

LGTM, I think we should also include an example of how to configure RabbitMQ, that would make it more convenient for users 😊

I would use this scheme as an example:

This way there is an .ENV varaible for the base DSN and it is possible to use multiple queues.

Alternatively, I can document it in a simplified way so that the .ENV contains the queue directly and therefore all tasks run into it (I find it not recommend but possibly easier and more error-free to understand for the usual user).

@brusch
Copy link
Member

brusch commented Apr 2, 2024

For me it would be fine that we just reference to the skeleton file.
So I'd then finish pimcore/skeleton#187 first and then update this PR.

@brusch
Copy link
Member

brusch commented Apr 2, 2024

Btw. just released v3.2 of our docker image with AMQP support ... just building at the moment:
https://github.com/pimcore/docker/actions/runs/8520253593

Copy link
Member

@fashxp fashxp left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@skoch98
Copy link
Contributor Author

skoch98 commented Apr 2, 2024

can we also make that statement consistent: https://github.com/pimcore/pimcore/blob/11.x/doc/01_Getting_Started/02_Advanced_Installation_Topics/01_Symfony_Messenger.md?plain=1#L22

For pimcore_core sure. But personally, I would leave the failed queue in the database so that it is easier to view it and report it if there are errors. What is your vendor view on this?

@brusch
Copy link
Member

brusch commented Apr 2, 2024

@skoch98 yes, I'd leave failed queue in db as well 👍

@skoch98
Copy link
Contributor Author

skoch98 commented Apr 3, 2024

I updated the docs but please make sure that pimcore/skeleton#187 will merged first so that the link is valid :-)

Edit: I think this is better for everyone: 4803044

Copy link

sonarqubecloud bot commented Apr 3, 2024

Quality Gate Passed Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
No data about Coverage
No data about Duplication

See analysis details on SonarCloud

@skoch98 skoch98 requested a review from jdreesen April 5, 2024 07:04
@mcop1 mcop1 self-assigned this Apr 19, 2024
@mcop1 mcop1 modified the milestones: 11.2.3, 11.3.0 Apr 22, 2024
@mcop1
Copy link
Contributor

mcop1 commented Apr 22, 2024

Since pimcore/docker#152 was merged, the image was released and the pr looks good, I merged it.
Thanks everyone for creating/improving the pr 👍

@mcop1 mcop1 merged commit 92a307f into pimcore:11.x Apr 22, 2024
4 checks passed
@github-actions github-actions bot locked and limited conversation to collaborators Apr 22, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants