rabbitmq / rabbitmq-common

Common library used by rabbitmq-server and rabbitmq-erlang-client
Other
66 stars 112 forks source link

Allow specifying a config file for start-cluster #362

Closed lukebakken closed 4 years ago

lukebakken commented 4 years ago

@dumbbell this is probably not the correct way to pass this env variable on, but it works.

What do you think?

dumbbell commented 4 years ago

Why do you need to do this change?

If you specify a variable on the make(1) command line, it will be automatically exported and thus passed to all child processes. Thus, running the following command should yield the expected result:

make start-background-broker RABBITMQ_CONFIG_FILE=/path/to/file.conf

You can even use the shorter name ($CONFIG_FILE).

lukebakken commented 4 years ago

It would be nice to use the same config file for all nodes when make start-cluster is used.

Without the change in this PR, make RABBITMQ_CONFIG_FILE=/path/to/file.conf NODES=3 start-cluster does not pass RABBITMQ_CONFIG_FILE to each cluster node.

Note, this may not be the right way to add support for RABBITMQ_CONFIG_FILE to start-cluster, but it does work.

dumbbell commented 4 years ago

You're right, there is a problem with our start-{brokers,cluster} or start-background-broker recipes, the variable does not make it to the sub-recipe...

The GNU Make documentation says it should and a quick test with a simple Makefile confirmed that. So our recipes break this mechanism. I'm looking into it.

dumbbell commented 4 years ago

The following patch "fixes" the problem:

diff --git a/mk/rabbitmq-run.mk b/mk/rabbitmq-run.mk
index 069be9b..5855a5d 100644
--- a/mk/rabbitmq-run.mk
+++ b/mk/rabbitmq-run.mk
@@ -275,7 +275,7 @@ DIST_TARGET ?= test-dist
 endif
 endif

-run-broker run-tls-broker: RABBITMQ_CONFIG_FILE ?= $(basename $(TEST_CONFIG_FILE))
+run-broker run-tls-broker: RABBITMQ_CONFIG_FILE := $(basename $(TEST_CONFIG_FILE))
 run-broker:     config := $(test_rabbitmq_config)
 run-tls-broker: config := $(test_rabbitmq_config_with_tls)
 run-tls-broker: $(TEST_TLS_CERTS_DIR)

This looks like a bug in GNU Make, but the patch does not cause regression.

I will commit and push it with other improvements.

lukebakken commented 4 years ago

Thanks @dumbbell - feel free to use this branch of course!

dumbbell commented 4 years ago

I pushed directly to master already.