-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Fix incorrect path for RABBITMQ_CONFIG_FILE #5184
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
Conversation
|
Doc generation appears to be failing but it appears to be unrelated since its some issue with Python on deploying docs, i.e. |
kiview
left a comment
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?
| 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.
| 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. |
kiview
left a comment
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_FILEwas pointing to an incorrect path (i.e."/etc/rabbitmq/rabbitmq-custom"rather than"/etc/rabbitmq/rabbitmq-custom.conf") which meant that thewithEnvdidn'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.confand not the contents. Due to this the test/s were updated to make sure the actual contents of/etc/rabbitmq/rabbitmq-custom.confis the same as theMountableFilethat gets passed in (the other related tests have also been updated to make sure that they work as expected)