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
Fix incorrect path for RABBITMQ_CONFIG_FILE #5184
Fix incorrect path for RABBITMQ_CONFIG_FILE #5184
Conversation
Doc generation appears to be failing but it appears to be unrelated since its some issue with Python on deploying docs, i.e.
|
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.
Great catch @mdedetrich and thanks for providing this PR!
The docs error is indeed unrelated to this PR, we will fix this in #5186.
The only thing I would like us to reconsider is if we can somehow test that the configured and copied config file comes into effect, rather than testing for the implementation detail of it being present in a certain location of the container. Do you have an idea for using a specific config somewhere, that would allow for this?
container.withRabbitMQConfigErlang(MountableFile.forClasspathResource("/rabbitmq-custom.config")); | ||
container.start(); | ||
|
||
assertThat(container.getLogs()).contains("config file(s) : /etc/rabbitmq/rabbitmq-custom.config"); | ||
assertThat(container.execInContainer("cat", "/etc/rabbitmq/rabbitmq-custom.config") | ||
.getStdout() | ||
).contains(configContents); |
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.
Checking for the content here does not really align that the file name and the ENV value are consistent.
container.start(); | ||
|
||
assertThat(container.getLogs()).contains("config file(s) : /etc/rabbitmq/rabbitmq-custom.conf"); | ||
assertThat(container.execInContainer("cat", "/etc/rabbitmq/rabbitmq-custom.conf") | ||
.getStdout() | ||
).contains(configContents); |
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 think this is tricky to check for the content here, because it checks for the side-effect of an implementation detail (if file at RABBITMQ_CONFIG_FILE
does not exist, on startup a default rabbitmq-custom.conf
is written?).
That's why it will fail without the fix here with:
java.lang.AssertionError:
Expecting actual:
"loopback_users.guest = false
listeners.tcp.default = 5672
default_pass = guest
management.tcp.port = 15672
"
to contain:
"loopback_users.guest = false
listeners.tcp.default = 5555
"
I changed the tests to check if the effect of changing the log level is applied through them. |
Sorry for taking so long, I was a bit busy. @kiview are more things required? tbh I am not that familiar with rabbitmq and this is just something I have noticed. |
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.
Np, I just adopted the tests accordingly, thanks for your contribution 👍
Fixes a bug where the
RABBITMQ_CONFIG_FILE
was pointing to an incorrect path (i.e."/etc/rabbitmq/rabbitmq-custom"
rather than"/etc/rabbitmq/rabbitmq-custom.conf"
) which meant that thewithEnv
didn't end up doing anything since it was pointing to a path rather than a file. The tests didn't pick up this bug because they were just testing the existence of/etc/rabbitmq/rabbitmq-custom.conf
and not the contents. Due to this the test/s were updated to make sure the actual contents of/etc/rabbitmq/rabbitmq-custom.conf
is the same as theMountableFile
that gets passed in (the other related tests have also been updated to make sure that they work as expected)