Remove defaults and add manual flag parsing for configuration #100
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR removes default values for critical Neo4j configuration parameters (URI, username, password) and introduces CLI flag parsing to allow command-line overrides of environment variables. The changes shift from implicit defaults to explicit configuration requirements, with validation ensuring all required parameters are provided.
Key changes:
- CLI flags (
--neo4j-uri,--neo4j-username,--neo4j-password,--neo4j-database) now override environment variables LoadConfig()no longer provides defaults for URI, username, and password, making them required- Enhanced test coverage including integration tests for real Neo4j connectivity and missing environment variable scenarios
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/config/config.go |
Removed defaults for URI/username/password; added GetEnv() function; LoadConfig() no longer validates or returns errors |
cmd/neo4j-mcp/main.go |
Added flag parsing for Neo4j configuration with CLI flag precedence over environment variables |
internal/cli/args.go |
Implemented manual flag validation in HandleArgs() to allow configuration flags to pass through to the flag package |
internal/cli/args_test.go |
Added test cases for configuration flag parsing including missing value scenarios |
internal/config/config_test.go |
Refactored unit tests to use t.Setenv() and test valid/invalid configuration scenarios separately |
test/integration/config/config_test.go |
Added integration tests for Neo4j connectivity and missing environment variable handling |
.github/workflows/build-and-test.yml |
Added missing NEO4J_USERNAME environment variable for integration tests |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <[email protected]>
Co-authored-by: Copilot <[email protected]>
remove redundant comment
mjfwebb
left a comment
mjfwebb
left a comment
There was a problem hiding this comment.
Very nice! Left some comments for your consideration 👍
| --neo4j-uri <URI> Neo4j connection URI (overrides env var) | ||
| --neo4j-username <USERNAME> Database username (overrides env var) | ||
| --neo4j-password <PASSWORD> Database password (overrides env var) | ||
| --neo4j-database <DATABASE> Database name (overrides env var) | ||
There was a problem hiding this comment.
I think I would like to be a bit more explicit here
| --neo4j-uri <URI> Neo4j connection URI (overrides env var) | |
| --neo4j-username <USERNAME> Database username (overrides env var) | |
| --neo4j-password <PASSWORD> Database password (overrides env var) | |
| --neo4j-database <DATABASE> Database name (overrides env var) | |
| --neo4j-uri <URI> Neo4j connection URI (overrides environment variable NEO4J_URI) | |
| --neo4j-username <USERNAME> Database username (overrides environment variable NEO4J_USERNAME) | |
| --neo4j-password <PASSWORD> Database password (overrides environment variable NEO4J_PASSWORD | |
| --neo4j-database <DATABASE> Database name (overrides environment variable NEO4J_DATABASE) | |
| NEO4J_TELEMETRY Enable/disable telemetry (default: true) | ||
| NEO4J_READ_ONLY Enable read-only mode (default: false) |
There was a problem hiding this comment.
We don't want these to be args?
| /* | ||
| Example walkthrough for argument parsing: | ||
| neo4j-mcp --neo4j-uri bolt://localhost:7687 --neo4j-username test | ||
| os.Args: | ||
| - os.Args[0] = "neo4j-mcp" | ||
| - os.Args[1] = "--neo4j-uri" | ||
| - os.Args[2] = "bolt://localhost:7687" | ||
| - os.Args[3] = "--neo4j-username" | ||
| - os.Args[4] = "test" | ||
| As the loop processes: | ||
| 1. i=1: Matches case "--neo4j-uri" → i += 2 → i=3 (skips the URI value) | ||
| 2. i=3: Matches case "--neo4j-username" → i += 2 → i=5 (skips the "test" value) | ||
| 3. i=5: Loop ends | ||
| This allows the arguments to "pass through" untouched so that flag.Parse() in main.go can later handle them properly. | ||
| */ | ||
|
|
There was a problem hiding this comment.
Maybe it's just me but I find this unnecessarily verbose, when the code itself also explains how this works.
| var MixPanelToken = "" | ||
|
|
||
| func main() { | ||
| // Define CLI flags for configuration |
There was a problem hiding this comment.
Just a personal choice here, but I would have likely put all of these code additions into config.LoadConfig() instead of polluting main()
There was a problem hiding this comment.
IMO, here we should delegate the "cli" and "config" with clear separation of ownership, from the main.go file I would keep the consumer logic as dummy as possible:
- "handleArgs" we could return a typed struct with the expected args so that can be used later by "loadConfig"
- "loadConfig" that returns errors in case of validation errors
|
|
||
| import ( | ||
| "context" | ||
| "flag" |
There was a problem hiding this comment.
what a nice std library
MacondoExpress
left a comment
MacondoExpress
left a comment
There was a problem hiding this comment.
Nice, left some comments,
please when the comments are addressed also remember to update the documentation:
- Contributing
- Readme
- User facing documentation
| cfg := config.LoadConfig() | ||
|
|
||
| // Override config with CLI flags if provided (CLI flags take precedence) | ||
| if *neo4jURI != "" { |
There was a problem hiding this comment.
Same as @mjfwebb pointed out, I would delegate this "overwrite" to the config package, same as "cfg.Validate()" I see no benefits to delegate the consumer to correctly invoke it, I would delegate that method to be consumed by config.loadConfig
| var MixPanelToken = "" | ||
|
|
||
| func main() { | ||
| // Define CLI flags for configuration |
There was a problem hiding this comment.
IMO, here we should delegate the "cli" and "config" with clear separation of ownership, from the main.go file I would keep the consumer logic as dummy as possible:
- "handleArgs" we could return a typed struct with the expected args so that can be used later by "loadConfig"
- "loadConfig" that returns errors in case of validation errors
| // | ||
| // If any of these environment variables are missing, the test will fail with a clear error message. | ||
| // If the environment variables are set but the database is not accessible, the test will fail with a connection error. | ||
| func TestLoadConfig_Neo4jConnectivity(t *testing.T) { |
There was a problem hiding this comment.
Let's remove this test, as we can test entirely with unit testing, without undesired side effects!
Introduce manual parsing for CLI flags related to Neo4j configuration, allowing users to override environment variables. Enhance test coverage for edge cases and integration tests with real environment variables.