Make directory for CloudConfig templates configurable - #65
Conversation
|
|
|
/easycla |
|
/easycla |
701aea5 to
bedfbc8
Compare
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
bedfbc8 to
78e0f55
Compare
| OPTIONAL_VARS = { | ||
| debug_mode: 'BAT_DEBUG_MODE', | ||
| cc_templates_dir: 'BAT_CC_TEMPLATES_DIR', | ||
| }.freeze |
|
An update, for my part -- I've got it running against the LXD CPI with the BAT_CC_TEMPLATES_DIR environment variable and it seems to be running just fine! Note that the README should also be updated as well. |
78e0f55 to
e64e438
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@lib/bat/cloud_config.rb`:
- Around line 71-80: Update cc_templates_dir to stop checking Dir.exist? for an
explicitly configured `@env.cc_templates_dir`. Preserve the existing non-empty
configuration check, return the configured path as-is, and retain
DEFAULT_TEMPLATES_DIR only when no path is configured.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 2dbb0c82-2133-47eb-be5a-70d3d341ddfb
📒 Files selected for processing (6)
README.mdlib/bat/cloud_config.rblib/bat/env.rblib/bat/requirements.rbspec/bat/requirements_spec.rbspec/system/spec_helper.rb
|
|
||
| def cc_templates_dir | ||
| if @env.cc_templates_dir && | ||
| @env.cc_templates_dir != "" && | ||
| Dir.exist?(@env.cc_templates_dir) | ||
| @env.cc_templates_dir | ||
| else | ||
| DEFAULT_TEMPLATES_DIR | ||
| end | ||
| end |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Avoid silent fallbacks for explicit configurations.
If BAT_CC_TEMPLATES_DIR is explicitly set to a non-existent path, Dir.exist? evaluates to false and the code silently falls back to DEFAULT_TEMPLATES_DIR. This can lead to confusing test behavior where the default templates are used instead of custom ones, without warning the user.
Removing the Dir.exist? check allows File.read downstream to fail loudly with an Errno::ENOENT error indicating the exact path it attempted to read from, making misconfigurations much easier to debug.
♻️ Proposed fix
def cc_templates_dir
- if `@env.cc_templates_dir` &&
- `@env.cc_templates_dir` != "" &&
- Dir.exist?(`@env.cc_templates_dir`)
+ if `@env.cc_templates_dir` && !`@env.cc_templates_dir.empty`?
`@env.cc_templates_dir`
else
DEFAULT_TEMPLATES_DIR
end
end📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def cc_templates_dir | |
| if @env.cc_templates_dir && | |
| @env.cc_templates_dir != "" && | |
| Dir.exist?(@env.cc_templates_dir) | |
| @env.cc_templates_dir | |
| else | |
| DEFAULT_TEMPLATES_DIR | |
| end | |
| end | |
| def cc_templates_dir | |
| if `@env.cc_templates_dir` && !`@env.cc_templates_dir.empty`? | |
| `@env.cc_templates_dir` | |
| else | |
| DEFAULT_TEMPLATES_DIR | |
| end | |
| end |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@lib/bat/cloud_config.rb` around lines 71 - 80, Update cc_templates_dir to
stop checking Dir.exist? for an explicitly configured `@env.cc_templates_dir`.
Preserve the existing non-empty configuration check, return the configured path
as-is, and retain DEFAULT_TEMPLATES_DIR only when no path is configured.
See #64