Repository navigation
config: detect duplicate & internal job names at parse time - #908
Merged
Merged
Conversation
Before this PR, config parsing would accept duplicate job names. `zrepl daemon` would later fail to start with a panic. But tools like `zrepl configcheck` would pass. This PR adds a check to ensure job names are unique. Similarly, internal job names were not being rejected by config parsing Move that check to parse-time as well.
There was a problem hiding this comment.
Pull request overview
This PR moves validation of duplicate job names and internal job names (those starting with "_") from daemon runtime to configuration parse time. This improvement allows tools like zrepl configcheck to catch these errors without starting the daemon.
Changes:
- Moved
IsInternalJobNamefunction fromdaemonpackage toconfigpackage for better accessibility - Added
validateJobNamesfunction in config parsing that checks for both internal job names and duplicates after config includes are expanded - Added comprehensive test coverage with three invalid sample configs
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| internal/config/config.go | Added IsInternalJobName function and validateJobNames that validates job names after includes are expanded |
| internal/daemon/daemon.go | Removed IsInternalJobName function and updated all calls to use config.IsInternalJobName |
| internal/client/status/viewmodel/render.go | Updated import and function call to use config.IsInternalJobName |
| internal/config/config_test.go | Added TestInvalidSampleConfigsFailToParse to verify invalid configs are rejected |
| internal/config/config_include_test.go | Removed redundant tests that are covered by existing TestSampleConfigsAreParsedWithoutErrors |
| internal/config/samples/invalid/* | Added three test cases: internal job name, duplicate job names, and duplicate job names with includes |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Before this PR, config parsing would accept duplicate job names.
zrepl daemonwould later fail to start with a panic. But tools likezrepl configcheckwould pass.This PR adds a check to ensure job names are unique.
Similarly, internal job names were not being rejected by config parsing Move that check to parse-time as well.
Last, drive-by change: remove
internal/config/config_include_test.gointroduced in #856 . These aren't necessary because there is already a wildcard test for all valid configs.This PR adds the complimentary "invalid config" wildcard test.