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.

Sign up for GitHub

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

Jump to bottom

Add Enqueue recipes. #16

Merged
merged 9 commits into from
May 24, 2017
Merged

Add Enqueue recipes. #16

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
9 commits
Select commit Hold shift + click to select a range
ae9a9f6
Enqueue recipes.
makasim May 8, 2017
3ad516f
fix env default value
makasim May 11, 2017
1e47192
remove gitignore rule for fs transport.
makasim May 11, 2017
f432413
remove post install text.
makasim May 11, 2017
0911f27
Address review comments.
makasim May 12, 2017
fbd9322
upd
makasim May 12, 2017
e1e0723
upd
makasim May 12, 2017
0fc35f2
upd
makasim May 13, 2017
95f1347
upd
makasim May 15, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions enqueue/amqp-ext/0.3/etc/packages/enqueue_amqp.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
enqueue:
transport:
default: 'amqp'
amqp:
host: '%env(ENQUEUE_AMQP_HOST)%'
port: '%env(ENQUEUE_AMQP_PORT)%'
login: '%env(ENQUEUE_AMQP_LOGIN)%'
password: '%env(ENQUEUE_AMQP_PASSWORD)%'
vhost: '%env(ENQUEUE_AMQP_VHOST)%'
Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed


# If you use RabbitMQ broker you can replace amqp transport with this one. It enables specific features like priorities, delays.
# default: 'rabbitmq_amqp'
# rabbitmq_amqp:
#
# host: '%env(ENQUEUE_AMQP_HOST)%'
# port: '%env(ENQUEUE_AMQP_PORT)%'
# login: '%env(ENQUEUE_AMQP_LOGIN)%'
# password: '%env(ENQUEUE_AMQP_PASSWORD)%'
# vhost: '%env(ENQUEUE_AMQP_VHOST)%'
Copy link
Member

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

#
# # The option tells whether RabbitMQ broker has delay plugin installed or not
# delay_plugin_installed: false

12 changes: 12 additions & 0 deletions enqueue/amqp-ext/0.3/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"copy-from-recipe": {
"etc/": "%ETC_DIR%/"
},
"env": {
"ENQUEUE_AMQP_HOST": "localhost",
"ENQUEUE_AMQP_PORT": "5672",
"ENQUEUE_AMQP_LOGIN": "guest",
"ENQUEUE_AMQP_PASSWORD": "guest",
"ENQUEUE_AMQP_VHOST": "/"
}
}
3 changes: 3 additions & 0 deletions enqueue/enqueue-bundle/0.3/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
consume:
@test -f bin/console && bin/console enqueue:consume --setup-broker || echo "cannot consume messages (needs symfony/console)"
.PHONY: consume
Copy link
Member

Choose a reason for hiding this comment

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

I think this can be removed as well. We don't want to wrap all Symfony CLI commands as make tasks. We are using Makefile tasks for when this is not practical or not possible to use the Symfony CLI.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

6 changes: 6 additions & 0 deletions enqueue/enqueue-bundle/0.3/etc/packages/test/enqueue.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
enqueue:
transport:
default: 'null'
Copy link
Member

Choose a reason for hiding this comment

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

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

'null': ~
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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'

Copy link
Member

Choose a reason for hiding this comment

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

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!

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.

Copy link
Member

Choose a reason for hiding this comment

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

Noop ? Blackhole ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Is it a blocker?

Copy link
Contributor

Choose a reason for hiding this comment

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

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?

client:
traceable_producer: true
8 changes: 8 additions & 0 deletions enqueue/enqueue-bundle/0.3/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,8 @@
{
"bundles": {
"Enqueue\\Bundle\\EnqueueBundle": ["all"]
},
"copy-from-recipe": {
"etc/": "%ETC_DIR%/"
}
}
1 change: 1 addition & 0 deletions enqueue/enqueue-bundle/0.3/post_install.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
[Enqueue] Run <comment>make consume</> command to consume messages
Copy link
Member

Choose a reason for hiding this comment

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

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.

5 changes: 5 additions & 0 deletions enqueue/fs/0.3/etc/packages/enqueue_fs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
enqueue:
transport:
default: 'fs'
fs:
store_dir: '%env(ENQUEUE_FS_STORAGE)%'
12 changes: 12 additions & 0 deletions enqueue/fs/0.3/manifest.json
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
{
"copy-from-recipe": {
"etc/": "%ETC_DIR%/",
"var/": "%VAR_DIR%"
Copy link
Member

Choose a reason for hiding this comment

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

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

},
"env": {
"ENQUEUE_FS_STORAGE": "%VAR_DIR%/queue"
Copy link
Member

Choose a reason for hiding this comment

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

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?

},
"gitignore": [
"/var/queue"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@makasim makasim May 11, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

btw, your own enqueue-bundle depends on FrameworkBundle 😄

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I overcomplicated things. fixed.

]
}
Empty file added enqueue/fs/0.3/var/queue/.gitkeep
View file
Open in desktop
Empty file.