Conversation
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
…efault_role Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
|
| GitGuardian id | GitGuardian status | Secret | Commit | Filename | |
|---|---|---|---|---|---|
| 17671777 | Triggered | Generic Password | 372801e | tests/e2e/cluster-lifecycle/crc_install_delete/conftest.py | View secret |
🛠 Guidelines to remediate hardcoded secrets
- Understand the implications of revoking this secret by investigating where it is used in your code.
- Replace and store your secret safely. Learn here the best practices.
- Revoke and rotate this secret.
- If possible, rewrite git history. Rewriting git history is not a trivial act. You might completely break other contributing developers' workflow and you risk accidentally deleting legitimate data.
To avoid such incidents in the future consider
- following these best practices for managing and storing secrets including API keys and other credentials
- install secret detection on pre-commit to catch secret before it leaves your machine and ease remediation.
🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.
WalkthroughThe changes refactor the handling of default component directories in both code and configuration, moving from single string paths to pluralized keys holding lists of directories. Environment variable substitution in paths is now supported. Tests and configuration files are updated to use the new pluralized keys and list structures. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Initializer
participant OS/Env
User->>Initializer: initialize()
Initializer->>OS/Env: Read config (pluralized *_dirs keys)
loop For each default directory list
Initializer->>OS/Env: Replace ${VAR} in paths
Initializer->>OS/Env: Check if directory exists
alt Directory exists
Initializer->>Initializer: List components from directory
else Directory does not exist
Initializer->>Initializer: Skip directory
end
end
Initializer-->>User: Initialization complete
Suggested labels
Poem
✨ 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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (4)
src/core/initializer.py (1)
147-166: Minor: simplifyreplace_env_varscontrol flowAfter the early
returninside thestrbranch, the followingelifis unnecessary; convert toif …: return …/if …: return …/return valuefor clarity.tests/fvt/core/test_initizlizer.py (2)
105-108: Path building should useos.path.jointhroughout
config_data["default_roles_dirs"] + "/foo"hard-codes a separator; useos.path.join(config_data["default_roles_dirs"][0], "foo")for portability.
114-122:os.path.existsalways-true mock may hide regressionsReturning
Truefor every path means non-existent directories or mistakes inreplace_env_varswon’t be caught. Consider restricting the stub to the paths you intend to be valid.tests/custom-context.json (1)
101-110: Hard-coded absolute paths reduce portabilityThe pluralised default directory arrays embed user-specific absolute paths. Consider environment variables (already supported by
replace_env_vars) or relative paths to keep the sample context repo-agnostic.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (4)
src/core/initializer.py(2 hunks)src/core/internal_config.yaml(1 hunks)tests/custom-context.json(4 hunks)tests/fvt/core/test_initizlizer.py(2 hunks)
🧰 Additional context used
🪛 Pylint (3.3.7)
src/core/initializer.py
[refactor] 148-165: Unnecessary "elif" after "return", remove the leading "el" from "elif"
(R1705)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: kind-cluster-lifecycle-test (3.12)
- GitHub Check: non-cluster-test (3.12)
- GitHub Check: cluster-test (3.12)
🔇 Additional comments (2)
tests/fvt/core/test_initizlizer.py (1)
93-101: Keep overridden config values as listsProduction code now expects the pluralised keys to be lists; overriding them with a plain string diverges from real-world usage and hides bugs that could appear with multiple directories.
-config_data["default_roles_dirs"] = os.path.join(... ) +config_data["default_roles_dirs"] = [os.path.join(...)]Apply the same pattern to units & playbooks.
src/core/internal_config.yaml (1)
8-16: Missing in-repo default for units & playbooks?
default_roles_dirsstill covers in-tree components (src/roles), butdefault_units_dirs/default_playbooks_dirshave no equivalent (src/units,src/playbooks).
If in-repo defaults are required, add them to avoid relying solely on${UNOFFICIAL_COMPONENTS}.
| for key in [ | ||
| "default_roles_dirs", | ||
| "default_units_dirs", | ||
| "default_playbooks_dirs", | ||
| ]: | ||
| if key in self.config_data: | ||
| # First ensure it's a list | ||
| if not isinstance(self.config_data[key], list): | ||
| self.config_data[key] = [self.config_data[key]] | ||
|
|
||
| # Then replace environment variables in each path | ||
| self.config_data[key] = [ | ||
| self.replace_env_vars(path) for path in self.config_data[key] | ||
| ] | ||
|
|
||
| # Finally add loopy_root_path to each path | ||
| self.config_data[key] = [ | ||
| f"{self.loopy_root_path}/{path}" for path in self.config_data[key] | ||
| ] | ||
|
|
There was a problem hiding this comment.
Absolute-path duplication risk & OS-agnostic join
Blindly prefixing self.loopy_root_path with f"{self.loopy_root_path}/{path}" will corrupt already-absolute paths (e.g. /tmp/foo → /repo//tmp/foo) and forces POSIX separators.
Guard with os.path.isabs() and use os.path.join / os.path.normpath instead:
- self.config_data[key] = [
- f"{self.loopy_root_path}/{path}" for path in self.config_data[key]
- ]
+ self.config_data[key] = [
+ os.path.normpath(
+ os.path.join(self.loopy_root_path, path)
+ )
+ if not os.path.isabs(path) else os.path.normpath(path)
+ for path in self.config_data[key]
+ ]📝 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.
| for key in [ | |
| "default_roles_dirs", | |
| "default_units_dirs", | |
| "default_playbooks_dirs", | |
| ]: | |
| if key in self.config_data: | |
| # First ensure it's a list | |
| if not isinstance(self.config_data[key], list): | |
| self.config_data[key] = [self.config_data[key]] | |
| # Then replace environment variables in each path | |
| self.config_data[key] = [ | |
| self.replace_env_vars(path) for path in self.config_data[key] | |
| ] | |
| # Finally add loopy_root_path to each path | |
| self.config_data[key] = [ | |
| f"{self.loopy_root_path}/{path}" for path in self.config_data[key] | |
| ] | |
| for key in [ | |
| "default_roles_dirs", | |
| "default_units_dirs", | |
| "default_playbooks_dirs", | |
| ]: | |
| if key in self.config_data: | |
| # First ensure it's a list | |
| if not isinstance(self.config_data[key], list): | |
| self.config_data[key] = [self.config_data[key]] | |
| # Then replace environment variables in each path | |
| self.config_data[key] = [ | |
| self.replace_env_vars(path) for path in self.config_data[key] | |
| ] | |
| # Finally add loopy_root_path to each path | |
| self.config_data[key] = [ | |
| os.path.normpath( | |
| os.path.join(self.loopy_root_path, path) | |
| ) | |
| if not os.path.isabs(path) else os.path.normpath(path) | |
| for path in self.config_data[key] | |
| ] |
🤖 Prompt for AI Agents
In src/core/initializer.py around lines 111 to 130, the code prefixes
self.loopy_root_path to each path using string concatenation, which can corrupt
absolute paths and is not OS-agnostic. Fix this by checking if each path is
absolute using os.path.isabs(); if it is not absolute, join it with
self.loopy_root_path using os.path.join, then normalize the result with
os.path.normpath to ensure correct path formatting across OSes.
|
/lgtm |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Tests