Add Enqueue recipes. #16

makasim · 2017-05-08T08:10:29Z

Q	A
License	MIT

This pull request adds a Symfony Flex recipe for Enqueue bundle and two transports filesystem and amqp.

Enqueue provides solutions to deal with message queues.

Pierstoval · 2017-05-08T21:13:52Z

enqueue/amqp-ext/0.3/etc/packages/enqueue-amqp.yaml

@@ -0,0 +1,23 @@
+enqueue:


(I posted a comment that I removed, don't mind it, I didn't see that there were more recipes than just one in this PR)

It seems that this recipe is made for enqueue/amqp-ext, but actually this config file is intended to be used together with the enqueue/enqueue-bundle package, so it seems a bit wrong to put this config file in here.

Maybe this shows something we could propose as a new feature for flex: if specific package is installed, install more configuration, else, just "suggest" it.

Actually, enqueue/enqueue-bundle package should suggest enqueue/amqp-ext if it's something that could be used with it.

Maybe this is something that could be improved both on Flex side ( => add config files depending on whether another package is installed or not) on your bundle side (suggest more extensions directly in the bundle so the user can know that other extensions exist for the EnqueueBundle bundle and that they are automatically (I didn't check so, I may be wrong by saying that) activated when using the bundle if the package is installed.

Imagine Bundle could benefit from such selective config, as well.

Maybe this shows something we could propose as a new feature for flex: if specific package is installed, install more configuration, else, just "suggest" it.

This might be a solution, though I support the point not to adding conditions to recipes.

The packs could be a solution to my problem. You do not install the bundle and amqp-ext separately but a enqueue/amqp-pack package. It installs the bundle and the transport and puts required configuration where needed

stof · 2017-05-11T15:32:38Z

enqueue/fs/0.3/manifest.json

+        "ENQUEUE_FS_STORAGE": "%VAR%/queue"
+    },
+    "gitignore": [
+        "/var/queue"


the whole content of var is already ignored in git, so this seems useless

Yeah, I know. It is done this way just for that case when framework bundle is not installed. The var folder ignore rule is added only when you install the framework bundle.

I could remove it if the case is not something we should worry about.

Framework bundle is a requirement, it cannot be uninstalled or not installed, so this is safe to remove.

btw, your own enqueue-bundle depends on FrameworkBundle 😄

I overcomplicated things. fixed.

stof · 2017-05-11T15:32:59Z

enqueue/fs/0.3/manifest.json

+        "var/": "%VAR_DIR%"
+    },
+    "env": {
+        "ENQUEUE_FS_STORAGE": "%VAR%/queue"


are you sure about %VAR% here ?

fabpot · 2017-05-11T15:45:31Z

enqueue/amqp-ext/0.3/etc/packages/enqueue_amqp.yaml

+            port: '%env(ENQUEUE_AMQP_PORT)%'
+            login: '%env(ENQUEUE_AMQP_LOGIN)%'
+            password: '%env(ENQUEUE_AMQP_PASSWORD)%'
+            vhost: '%env(ENQUEUE_AMQP_VHOST)%'


I've have had a look, but do you support a URL/DSN as well? That's what we are using for Doctrine and Swiftmailer, so that would be great if possible to do the same here as well.

agreed here. PaaS expose credentials as a URL, so this config is not usable drop-in on such PaaS.

fabpot · 2017-05-11T15:46:09Z

enqueue/enqueue-bundle/0.3/post_install.txt

@@ -0,0 +1 @@
+[Enqueue] Run <comment>make consume</> command to consume messages


I would remove this as the post install script if for things you need to do to make things work. It should not replace proper documentation of the package.

fabpot · 2017-05-11T15:47:00Z

enqueue/fs/0.3/manifest.json

+{
+    "copy-from-recipe": {
+        "etc/": "%ETC_DIR%/",
+        "var/": "%VAR_DIR%"


Is the var/queue/.gitkeep really needed? The package should be able to create it when it does not exist, right?

fabpot · 2017-05-11T15:50:19Z

enqueue/amqp-ext/0.3/etc/packages/enqueue_amqp.yaml

+#            port: '%env(ENQUEUE_AMQP_PORT)%'
+#            login: '%env(ENQUEUE_AMQP_LOGIN)%'
+#            password: '%env(ENQUEUE_AMQP_PASSWORD)%'
+#            vhost: '%env(ENQUEUE_AMQP_VHOST)%'


The whole config seems to be the same as the default one, except for the option. What about adding the comment on the default config with the explanation you have above?

The transport name (a key) is different: amqp and rabbitmq_amqp.

Then the If you use RabbitMQ broker ... part could just say "rename amqp with rabbitmq_amqp and the bundle will enable specific features ..."

fabpot · 2017-05-11T18:52:05Z

Did you close on purpose?

makasim · 2017-05-12T04:23:19Z

Did you close on purpose?

it was closed when I merged php-enqueue/enqueue-dev#78. I wrote there "fixes XXX".

javiereguiluz · 2017-05-12T14:16:18Z

enqueue/enqueue-bundle/0.3/etc/packages/test/enqueue.yaml

@@ -0,0 +1,6 @@
+enqueue:
+    transport:
+        default: 'null'


This 'null' value must be a string or could be a normal null value instead?

javiereguiluz · 2017-05-12T14:16:35Z

enqueue/enqueue-bundle/0.3/etc/packages/test/enqueue.yaml

+enqueue:
+    transport:
+        default: 'null'
+        'null': ~


I don't understand this line: 'null': ~

it defines the configuration for the NullTransport. But the key must be quoted in YAML, as unquoting it would parse it as null, not as the string 'null'

This is extremely confusing to me: a "default transport" which is a null string ... which then is a "null transport" with a null value. Can we think a better approach for this? A new config option for tests maybe? Thanks!

@javiereguiluz In this context 'null' is the name of the transport. The transport does not send nor receive anything. It is there just to mock a messaging API and useful in tests for example.

I've not come up with a better name for the transport.

Noop ? Blackhole ?

Maybe, but not now. That's a huge change and would take time. I'll do it later on.

Is it a blocker?

Having a "null" object is very common and is, of course, a well know design pattern; why are we trying to rename something that already describes exactly what the transport is?

javiereguiluz · 2017-05-12T14:18:01Z

enqueue/fs/0.3/manifest.json

+        "var/": "%VAR_DIR%"
+    },
+    "env": {
+        "ENQUEUE_FS_STORAGE": "%VAR_DIR%/queue"


Just asking. If the bundles is called "enqueue", should the var/ dir be called "enqueue/" too? The "queue" name is too generic ... what if someday Symfony releases a Queue component or bundle?

makasim · 2017-05-15T17:10:53Z

I address all the review comments but the PR still red. It says (@stof stof requested changes). What should I do to this?

fabpot · 2017-05-15T17:22:02Z

@stof needs to dismiss his review and someone else (including stof) need to approve the PR.

makasim · 2017-05-17T07:57:30Z

@stof needs to dismiss his review and someone else (including stof) need to approve the PR.

@stof I kindly remind you.

xabbuh · 2017-05-17T08:39:04Z

enqueue/fs/0.4/manifest.json

@@ -0,0 +1,5 @@
+{
+    "env": {
+        "ENQUEUE_DSN": "file:/%VAR_DIR%/enqueue"


Will the library take care to create the directory if it does not exist yet?

yes, it does. https://github.com/php-enqueue/enqueue-dev/blob/master/pkg/fs/FsContext.php#L43

fixed

javiereguiluz · 2017-05-24T06:17:19Z

enqueue/fs/0.4/manifest.json

@@ -0,0 +1,5 @@
+{
+    "env": {
+        "ENQUEUE_DSN": "file:/%VAR_DIR%/enqueue"


Shouldn't we use here file:// instead of file:/ ?

Isn't it a problem?
On Unix system, using file:// will result in paths like file:///var/www/dev/enqueue which might not work, whereas on windows it will result in file://e:/www/dev/enqueue which should work.

Dunno how to fix this 😱

I don't know about Linux, but on macOS file:///var/www/dev/enqueue not only works, but it's the only way this can work: 3 / after file: is normal.

Ok nevermind, I tested on my ubuntu distro and file:/// works, once again I should check before talking

I opened the issue. It'will be fixed.

symfony-bot deployed to staging May 8, 2017 08:10 View deployment

makasim changed the title ~~[WIP] Enqueue Fs pack recipe.~~ [WIP] Enqueue Bundle recipes. May 8, 2017

makasim mentioned this pull request May 8, 2017

The "https://pr16.contrib.flex.symfony.com/m/makasim/temp-file/0.2.0" file could not be downloaded. symfony/flex#74

Closed

symfony-bot deployed to staging May 8, 2017 11:55 View deployment

symfony-bot deployed to staging May 8, 2017 11:57 View deployment

symfony-bot deployed to staging May 8, 2017 12:11 View deployment

symfony-bot deployed to staging May 8, 2017 13:06 View deployment

symfony-bot deployed to staging May 8, 2017 13:11 View deployment

symfony-bot deployed to staging May 8, 2017 17:28 View deployment

symfony-bot deployed to staging May 8, 2017 17:57 View deployment

symfony-bot deployed to staging May 8, 2017 17:59 View deployment

makasim force-pushed the enqueue-recipes branch from 89954e0 to 0a7c03d Compare May 8, 2017 18:06

symfony-bot deployed to staging May 8, 2017 18:06 View deployment

makasim changed the title ~~[WIP] Enqueue Bundle recipes.~~ [WIP] Add Enqueue Bundle recipes. May 8, 2017

Pierstoval suggested changes May 8, 2017

View reviewed changes

makasim changed the title ~~[WIP] Add Enqueue Bundle recipes.~~ Add Enqueue recipes. May 11, 2017

Enqueue recipes.

ae9a9f6

makasim force-pushed the enqueue-recipes branch from 08e58b1 to ae9a9f6 Compare May 11, 2017 15:25

stof previously requested changes May 11, 2017

View reviewed changes

makasim added 2 commits May 11, 2017 18:36

fix env default value

3ad516f

remove gitignore rule for fs transport.

1e47192

fabpot reviewed May 11, 2017

View reviewed changes

makasim mentioned this pull request May 11, 2017

[fs] Filesystem transport must create a storage dir if it does not exists. php-enqueue/enqueue-dev#78

Merged

remove post install text.

f432413

symfony-bot deployed to staging May 11, 2017 16:34 View deployment

makasim closed this in php-enqueue/enqueue-dev#78 May 11, 2017

symfony-bot deployed to staging May 12, 2017 04:21 View deployment

makasim mentioned this pull request May 12, 2017

[amqp] Configure by string DSN. php-enqueue/enqueue-dev#80

Merged

javiereguiluz reviewed May 12, 2017

View reviewed changes

makasim mentioned this pull request May 12, 2017

[fs] add DSN support php-enqueue/enqueue-dev#82

Merged

makasim added 3 commits May 13, 2017 00:04

Address review comments.

0911f27

upd

fbd9322

upd

e1e0723

symfony-bot deployed to staging May 12, 2017 21:41 View deployment

upd

0fc35f2

symfony-bot deployed to staging May 13, 2017 06:12 View deployment

symfony-bot previously approved these changes May 13, 2017

View reviewed changes

This was referenced May 15, 2017

Add ability to set transport DSN directly to default transport factory. php-enqueue/enqueue-dev#81

Merged

[bundle] Set null transport as default. Prevent errors on bundle install. php-enqueue/enqueue-dev#77

Merged

upd

95f1347

makasim dismissed symfony-bot’s stale review via 95f1347 May 15, 2017 12:41

symfony-bot deployed to staging May 15, 2017 12:41 View deployment

symfony-bot approved these changes May 15, 2017

View reviewed changes

xabbuh reviewed May 17, 2017

View reviewed changes

fabpot approved these changes May 24, 2017

View reviewed changes

symfony-bot merged commit 1043324 into symfony:master May 24, 2017

javiereguiluz reviewed May 24, 2017

View reviewed changes

makasim deleted the enqueue-recipes branch May 24, 2017 06:45

makasim mentioned this pull request May 24, 2017

filesystem dsn must have one more / php-enqueue/enqueue-dev#99

Closed

robfrawley mentioned this pull request May 30, 2017

[Enqueue] Use simplified transport configuration liip/LiipImagineBundle#942

Merged

		@@ -0,0 +1 @@
		[Enqueue] Run <comment>make consume</> command to consume messages

Add Enqueue recipes. #16

Add Enqueue recipes. #16

Conversation

makasim commented May 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim May 11, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabpot commented May 11, 2017

makasim commented May 12, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented May 15, 2017

fabpot commented May 15, 2017

makasim commented May 17, 2017 • edited Loading

Choose a reason for hiding this comment

makasim May 17, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pierstoval May 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

makasim commented May 8, 2017 •

edited

Loading

makasim May 11, 2017 •

edited

Loading

makasim commented May 12, 2017 •

edited

Loading

makasim commented May 17, 2017 •

edited

Loading

makasim May 17, 2017 •

edited

Loading

Pierstoval May 24, 2017 •

edited

Loading