docker-entrypoint will now set default configs based on Neo4j Edition #192
Conversation
bfeshti
commented
Sep 11, 2019
bfeshti
commented
- Added the ability to set default configs based on Neo4j Edition.
- Added a test to check if the right configs are set correctly in the Enterprise edition.
- Added a test to check that there are no Enterprise edition configs set to the Community edition.
| @@ -228,14 +228,15 @@ fi | |||
| : ${NEO4J_dbms_connectors_default__advertised__address:=${NEO4J_dbms_connectors_defaultAdvertisedAddress:-}} | |||
| : ${NEO4J_ha_server__id:=${NEO4J_ha_serverId:-}} | |||
There was a problem hiding this comment.
HA configurations are also enterprise specific, these configs that contain ha should be inside your if statement. This applies to versions 3.3 and 3.4 of the docker entrypoint.
Since it was deprecated in 3.5 we shouldn't be setting HA configs (default or otherwise) in the entrypoints for 3.5 and onwards (they might be set in the conf file but there's not much we can do about that).
| ENTERPRISE=( | ||
| [causal_clustering.transaction_listen_address]="0.0.0.0:6000" | ||
| [causal_clustering.raft_listen_address]="0.0.0.0:7000" | ||
| [causal_clustering.discovery_listen_address]="0.0.0.0:5000" | ||
| ) | ||
|
|
||
| for conf in ${!ENTERPRISE[@]} ; do | ||
| for conf in ${!COMMUNITY[@]} ; do |
There was a problem hiding this comment.
Looks like you're missing the equivalent of this for for enterprise. Maybe a patching error?
| : ${NEO4J_causal__clustering_transaction__advertised__address:=$(hostname):6000} | ||
| : ${NEO4J_causal__clustering_raft__advertised__address:=$(hostname):7000} | ||
|
|
||
| if [ "${NEO4J_EDITION}" == "enterprise" ]; |
There was a problem hiding this comment.
why have this in a separate if to the enterprise stuff in lines 232-239? (this applies to the other entrypoints too)
| Stream<String> lines = Files.lines(logMount.resolve("debug.log")); | ||
| Optional<String> heapSizeMatch = lines.filter(s -> s.contains("dbms.memory.pagecache.size=512m")).findFirst(); | ||
| lines.close(); | ||
| Assertions.assertTrue(heapSizeMatch.isPresent(), "dbms.memory.pagecache.size was not set correctly"); | ||
| //Kill the container | ||
| container.stop(); | ||
| } | ||
|
|
||
| @Test | ||
| void testEnterpriseConfigsAreSetCorrectly() throws Exception { |
There was a problem hiding this comment.
My understanding of your change is that you're changing the default configuration settings so that for community edition, we're not setting a load of enterprise-only configurations.
This test is mounting a configuration file and checking that a enterprise only setting was read in. This doesn't test your change because the test doesn't look at default values, only that the configuration file (and its setting) gets read: which should always happen, even in community edition. This test is a duplicate of testReadTheConfFile.
What you should be testing is that:
- in enterprise edition, the enterprise only settings have the default value set. To test this is correct, you'd need to test a setting that's different in docker vs the configuration file like one of the settings that uses
$(hostname)egNEO4J_causal__clustering_raft__advertised__address - In community edition, the enterprise only settings aren't set at all. Your test
testCommunityConfigsAreSetCorrectlyalready does that so 👍
| container.stop(); | ||
| } | ||
| @Test | ||
| void testCommunityConfigsAreSetCorrectly() throws Exception { |
There was a problem hiding this comment.
The test name doesn't really describe what the test is checking either. Can you name it something like testCommunityDoesNotHaveEnterpriseConfigs or similar?
| Stream<String> lines = Files.lines(logMount.resolve("debug.log")); | ||
| Optional<String> heapSizeMatch = lines.filter(s -> s.contains("dbms.memory.pagecache.size=512m")).findFirst(); | ||
| lines.close(); | ||
| Assertions.assertTrue(heapSizeMatch.isPresent(), "dbms.memory.pagecache.size was not set correctly"); | ||
| //Kill the container | ||
| container.stop(); | ||
| } | ||
|
|
||
| @Test | ||
| void testEnterpriseConfigsAreSetCorrectly() throws Exception { |
There was a problem hiding this comment.
The test name doesn't really describe what the test is checking. When I/you look back on this in 2 or 3 months time we'd have to read all the code to know what is actually being tested. Can you name it something like testEnterpriseOnlyDefaultsConfigsAreSet or something similar?
| @@ -0,0 +1 @@ | |||
| causal_clustering.transaction_listen_address=0.0.0.0:9000 | |||
There was a problem hiding this comment.
You don't need this file any more I don't think
| container.start(); | ||
| //Read debug.log to check that causal_clustering confs are set successfully | ||
| Stream<String> lines = Files.lines(logMount.resolve("debug.log")); | ||
| Optional<String> ccPresent = lines.filter(s -> s.contains("causal_clustering.transaction_advertised_address")).findFirst(); |
There was a problem hiding this comment.
I think it would be better if we actually tested that the configuration is also set to the value we expect. It looks as though entrypoint resolves $(hostname) to be the first 12 digits of the container id, so this works for me:
String expectedTxAddress = container.getContainerId().substring( 0, 12 ) + ":6000";
Stream<String> lines = Files.lines(logMount.resolve("debug.log"));
Optional<String> ccPresent = lines.filter(s -> s.contains("causal_clustering.transaction_advertised_address="+expectedTxAddress)).findFirst();
lines.close();
