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

Make use of new database setting for determining conf directory #719

Merged
merged 4 commits into from
Jan 14, 2025

Conversation

JoelBergstrand
Copy link
Contributor

@JoelBergstrand JoelBergstrand commented Jan 8, 2025

Makes use of the new setting introduced in PR. This allows for the removal of the previously hacky way of determining the conf directory. Removes associated tests and updates tests that previously depended on the hack.

@JoelBergstrand JoelBergstrand force-pushed the 2025.01-make-use-of-conf-dir-setting branch from 74a0ae5 to 4856577 Compare January 8, 2025 10:45
build.gradle Outdated Show resolved Hide resolved
}
return neo4jConfFolder;
}
return neo4jConfig.get(configuration_directory).toString();
Copy link
Contributor

Choose a reason for hiding this comment

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

I can't comment directly on the correct line, but where this is used now has an incorrect comment, can you fix that up? :) also the log info is wrong now

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could the log say something like this?

Suggested change
return neo4jConfig.get(configuration_directory).toString();
String neo4jConfFolder = neo4jConfig.get(configuration_directory).toString();
log.info("from database setting: server.directories.configuration=%s", neo4jConfFolder);
return neo4jConfFolder;

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm actually, that line is about setting a system property. Do we even need that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't think we need the loging any more. Omitting it and updating the comment.

Copy link
Contributor

@loveleif loveleif left a comment

Choose a reason for hiding this comment

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

Super

@JoelBergstrand JoelBergstrand merged commit fb799e4 into dev Jan 14, 2025
11 checks passed
@JoelBergstrand JoelBergstrand deleted the 2025.01-make-use-of-conf-dir-setting branch January 14, 2025 15:05
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.

3 participants