feat: bypass validation for total reset #2941
Conversation
📝 WalkthroughWalkthroughA skipValidation flag has been threaded through the configuration system, allowing validation to be bypassed when the --force option is provided. The flag is propagated from the command interface through the configuration repository to individual Config instances, where conditional logic gates the schema validation. Changes
Sequence DiagramsequenceDiagram
participant User
participant BaseCommand
participant ConfigFileJsonRepository
participant Config
User->>BaseCommand: Execute command with --force flag
BaseCommand->>ConfigFileJsonRepository: read({ skipValidation: true })
rect rgba(144, 238, 144, 0.3)
Note over ConfigFileJsonRepository: skipValidation extracted from options
end
ConfigFileJsonRepository->>Config: constructor(name, options, skipValidation: true)
rect rgba(255, 182, 193, 0.3)
Note over Config: Validation conditional check
alt skipValidation = true
Config->>Config: Skip AJV schema validation
else skipValidation = false
Config->>Config: Perform AJV validation<br/>(throw on invalid)
end
end
Config-->>ConfigFileJsonRepository: Config instance
ConfigFileJsonRepository-->>BaseCommand: Loaded config
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
ktechmidas
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dashmate/src/config/Config.js (1)
130-145: Validation bypass is correctly implemented with appropriate scope limitations.The conditional validation logic correctly allows bypassing schema checks when
skipValidationis true. Importantly, theset()method (lines 78-111) still validates individual property changes, which limits the security impact to bulk loading scenarios only.The trade-off is acceptable for the reset use case, though there's a risk that invalid configurations could cause runtime errors later.
💡 Optional: Consider logging when validation is skipped
For observability and debugging, you might want to log when validation is bypassed:
setOptions(options, skipValidation = false) { const clonedOptions = lodashCloneDeep(options); if (!skipValidation) { const isValid = Config.ajv.validate(configJsonSchema, clonedOptions); if (!isValid) { const message = Config.ajv.errorsText(undefined, { dataVar: 'config' }); throw new InvalidOptionsError( clonedOptions, Config.ajv.errors, message, ); } + } else { + // Log or warn that validation is being skipped + console.warn(`Skipping validation for config '${this.name}' (--force mode)`); } this.options = clonedOptions;This would make it more visible when configs are loaded without validation, which could help with debugging if issues arise later.
📜 Review details
Configuration used: defaults
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
packages/dashmate/src/config/Config.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.jspackages/dashmate/src/oclif/command/BaseCommand.js
🧰 Additional context used
📓 Path-based instructions (2)
**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
**/*.{js,jsx,ts,tsx}: Use 2-space indent for JS/TS files
Use camelCase for variables and functions in JS/TS
Use PascalCase for class names in JS/TS
Use ESLint with Airbnb/TypeScript rules for JS/TS files
Files:
packages/dashmate/src/config/Config.jspackages/dashmate/src/oclif/command/BaseCommand.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.js
packages/**/!(node_modules)/**/*.{js,jsx,ts,tsx}
📄 CodeRabbit inference engine (AGENTS.md)
Use kebab-case for filenames within JS packages
Files:
packages/dashmate/src/config/Config.jspackages/dashmate/src/oclif/command/BaseCommand.jspackages/dashmate/src/config/configFile/ConfigFileJsonRepository.js
🧬 Code graph analysis (2)
packages/dashmate/src/config/Config.js (1)
packages/dashmate/src/config/obfuscateConfig.js (1)
clonedOptions(15-15)
packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js (1)
packages/dashmate/src/config/Config.js (1)
Config(16-186)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (4)
- GitHub Check: Build Docker images (RS-DAPI, rs-dapi, rs-dapi) / Build RS-DAPI image
- GitHub Check: Build Docker images (Dashmate helper, dashmate-helper, dashmate-helper) / Build Dashmate helper image
- GitHub Check: Build JS packages / Build JS
- GitHub Check: Rust crates security audit
🔇 Additional comments (3)
packages/dashmate/src/config/configFile/ConfigFileJsonRepository.js (1)
25-30: LGTM! Clean implementation of skipValidation parameter.The skipValidation flag is properly threaded through the read method to Config instantiation with appropriate defaults and documentation.
Also applies to: 65-65
packages/dashmate/src/config/Config.js (1)
20-26: LGTM! Constructor properly threads skipValidation parameter.The constructor signature is updated cleanly with appropriate defaults and documentation. The skipValidation flag is correctly passed to setOptions.
packages/dashmate/src/oclif/command/BaseCommand.js (1)
44-47: This concern is incorrect. Theforceflag is properly defined in command subclasses.The
forceflag is correctly defined in specific command classes that use it (e.g.,packages/dashmate/src/commands/reset.jsline 13,packages/dashmate/src/commands/restart.jsline 14, etc.). This is the correct oclif pattern—command-specific flags are defined in the command classes themselves, not in base classes. When these commands extendConfigBaseCommandorGroupBaseCommandand define their own flags, oclif merges all parent flags, makingthis.parsedFlags.forceavailable in the context where it's used. The code at lines 44-47 works as intended.Likely an incorrect or invalid review comment.
Issue being fixed or feature implemented
When we attempt to reset in order to redeploy, dashmate will check the config despite us sending --force
What was done?
Bypass config checks when sending a reset --hard --force
How Has This Been Tested?
Tested on devnet-tadi
Breaking Changes
None
Checklist:
For repository code-owners and collaborators only
Summary by CodeRabbit
--forceflag to skip configuration validation when necessary, providing flexibility for advanced users to bypass validation checks in specific scenarios.✏️ Tip: You can customize this high-level summary in your review settings.