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.

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

Add open options settings for test configs #320

Open
wants to merge 494 commits into
base: master
Choose a base branch
from

Conversation

MishaDemianenko
Copy link
Contributor

@MishaDemianenko MishaDemianenko commented Dec 1, 2021

Previously some module options were hardcoded in neo4j. Now they are part of distributes neo4j config. As a result, if config gets lost or changed for test purposes we need to make sure we have a minimal set of options that will actually allow neo4j to be started.

Bledi Feshti and others added 30 commits July 2, 2019 13:10
…configuration file and added some unit tests for that
…untime

Adding the ability to install neo4j labs plugins on container startup
…poc-at-runtime"

This reverts commit aef2eda, reversing
changes made to c832a61.
After the changes in this PR, there is no hard requirement to have to install a valid certificate before starting Neo4j.
merged latest changes on the private branch.
… being run on a finished container, which errored.
Moved 4.0 scripts into the public repository
This is to enable the tests to run inside a docker container, even though they themselves spawn new containers.
It was failing in our CI pipeline because of permission errors on mounted folders when testing mounting as non-root users.
…nstall-apoc-at-runtime""

This reverts commit 84425d9.

Which re-enables plugin installation at runtime
jennyowen and others added 22 commits October 21, 2021 13:58
This reverts commit f6a6d8b, reversing
changes made to 81a2edb.
The setting: dbms.memory.pagecache.size is about to change from
`Setting<String>`` to `Setting<Long>`` and will therefore change
slightly how the setting value is printed in the debug log.
This changes a test assertion so that it accepts both variants of
that debug log print.
Temporary disable upgrade tests since upgrade is under update for 5.0 and not working atm
Change pagecache size debug log output assertion due to type change
Copy link
Member

@jennyowen jennyowen left a comment

Choose a reason for hiding this comment

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

I really have no idea what feature you're trying to test here or why it requires editing existing tests for things. If it's a new feature, surely it needs a new test?

@@ -1 +1,3 @@
dbms.memory.heap.max_size=512m
Copy link
Member

Choose a reason for hiding this comment

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

you removed this important line

@@ -240,6 +242,9 @@ void testCantWriteIfSecureEnabledAndNoPermissions_logs() throws IOException
void canMountAllTheThings_fileMounts(boolean asCurrentUser) throws Exception
{
Path testOutputFolder = HostFileSystemOperations.createTempFolder( "mount-everything-" );
Path confFile = Paths.get( "src", "test", "resources", "confs", "MountConf.conf" );
Files.copy( confFile, testOutputFolder.resolve( "neo4j.conf" ) );

try(GenericContainer container = setupBasicContainer( asCurrentUser, false ))
{
HostFileSystemOperations.createTempFolderAndMountAsVolume( container, "conf", "/conf", testOutputFolder );
Copy link
Member

Choose a reason for hiding this comment

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

in this test it creates a folder /tmp/mount-everything-####/ then underneath on this line (L250) it creates a subfolder /tmp/mount-everything-####/conf which is where your conf file needs to go. At the moment you're not mounting it in a location where docker will see the conf.

@@ -268,7 +273,7 @@ void canMountAllTheThings_namedVolumes(boolean asCurrentUser) throws Exception
{
container.withCreateContainerCmdModifier(
(Consumer<CreateContainerCmd>) cmd -> cmd.getHostConfig().withBinds(
Bind.parse("conf-"+id+":/conf"),
// Bind.parse("conf-"+id+":/conf"), // todo we need a set of open options, ask
Copy link
Member

Choose a reason for hiding this comment

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

why?

causal_clustering.transaction_advertised_address=localhost:6060

dbms.jvm.additional=--add-opens=java.base/java.nio=ALL-UNNAMED
Copy link
Member

Choose a reason for hiding this comment

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

why is it necessary to add these lines to all conf files?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry i forgot to put some indication that its not ready oir review yet :) but more or less we moving options to required open modules to config instead of hardcoding them and without them you can't start neo4j

Previously some module options were hardcoded in neo4j. Now they are part of distributes neo4j config. As a result, if config gets lost or changed for test purposes we need to make sure we have a minimal set of options that will actually allow neo4j to be started.
@MishaDemianenko
Copy link
Contributor Author

@jennyowen i forced pushed some changes with some description as well. Also, there is one case with binds where I'm not sure how to adapt the test so will follow your suggestion there.

So in short those 3 module options that now part of custom configs are required to start neo4j and without them container will not gonna start. As result if we have any test with custom config or config directory mapping we need to adapt those configs to have those options. Otherwise neo will fail on startupt

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.