add crc cluster install/delete role and gitaction#44
add crc cluster install/delete role and gitaction#44github-actions[bot] merged 6 commits intomainfrom
Conversation
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.
WalkthroughThis update introduces new roles, scripts, configurations, and end-to-end tests for managing the lifecycle of OpenShift Local (CRC) clusters, alongside enhancements and refinements for KIND cluster workflows. It adds flexible Makefile options, specialized test markers, and improved environment handling, while also updating VSCode debugging settings and internal script logic. Changes
Sequence Diagram(s)sequenceDiagram
participant Tester
participant LoopyCLI
participant CRCInstallRole
participant CRCUninstallRole
participant CRC_CLI
participant OpenShiftCluster
Tester->>LoopyCLI: Run install role (crc-install)
LoopyCLI->>CRCInstallRole: Execute main.sh with env vars
CRCInstallRole->>CRC_CLI: Check status/setup/start cluster
CRC_CLI->>OpenShiftCluster: Start/initialize cluster
CRCInstallRole->>LoopyCLI: Export cluster info to env file
Tester->>LoopyCLI: Run uninstall role (crc-uninstall)
LoopyCLI->>CRCUninstallRole: Execute main.sh with env vars
CRCUninstallRole->>CRC_CLI: Stop/delete/cleanup cluster
CRC_CLI->>OpenShiftCluster: Stop/delete cluster resources
CRCUninstallRole->>LoopyCLI: Report operation result
Possibly related PRs
Suggested labels
Suggested reviewers
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 (
|
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
Signed-off-by: Jooho Lee <jlee@redhat.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
🔭 Outside diff range comments (1)
src/roles/clusters/kind/install/main.sh (1)
101-112: WriteCLUSTER_TYPEonly on success to avoid misleading downstream rolesWhen
result != 0we intentionally skip writingKUBECONFIG_PATH, yet we still
appendCLUSTER_TYPE=kind.
That creates an output file even for failed installs, signalling to subsequent
roles that a cluster exists.- fi - echo "CLUSTER_TYPE=kind" >> ${OUTPUT_ENV_FILE} + echo "CLUSTER_TYPE=kind" >> ${OUTPUT_ENV_FILE} +fiThis keeps the semantics: no output file unless the role succeeds.
🧹 Nitpick comments (22)
src/roles/clusters/kind/install/config.yaml (1)
62-64: Terminate file with a newline to satisfy YAML lintingyaml-lint is flagging “no new line character at the end of file”.
Add a trailing newline after line 64 to keep tooling quiet and avoid CI noise.- required: true \ No newline at end of file + required: true +tests/e2e/cluster-lifecycle/kind_install_delete/test_kind.py (1)
55-57: Minor: keep command array on one line for brevityThe line break isn’t harmful, but you can keep the list compact without
exceeding 120 chars to improve readability.src/commons/scripts/utils.sh (1)
53-54: Quote variable in command substitution
is_positiveconsumes its argument as a single word; wrap the variable in
quotes to avoid word-splitting or globbing whenSHOW_DEBUG_LOGis unset or
contains spaces.-if [[ $(is_positive $SHOW_DEBUG_LOG) == "0" ]]; then +if [[ $(is_positive "$SHOW_DEBUG_LOG") == "0" ]]; thenpytest.ini (1)
30-37: Marker additions are clear but please document themThe new
cluster_lifecycle_tests,kind, andopenshift_localmarkers look fine; however, the repo’s CONTRIBUTING/README still only lists the older markers. A quick doc touch-up will save future confusion.tests/e2e/commons/cluster-tests/test_py_utils_e2e.py (2)
212-214: Prefer boolean truthiness over identity checks
assert retry(...) is False/is Truerelies on the function returning the literal singletonTrue/False; if it ever starts returning any truthy/falsy value (e.g.0/1or a subclass ofbool) these tests will fail for the wrong reason.Minimal change:
-assert retry(...) is False +assert not retry(...)Same for the
is Truevariants.Also applies to: 244-251, 278-286
297-298: Minor: failure message can be tightened
pytest.fail(f"Cluster command failed: {e.stderr.decode() if e.stderr else str(e)}")is ok, bute.stderr.decode(errors="replace")guards against non-UTF8 bytes. Not blocking.hacks/download-cli.sh (1)
182-192:crcinstall path handling is brittle
tar -xJproduces a versioned directory (crc-linux-<ver>-amd64).
If two different versions exist in/tmp/loopy_temp, the wildcard inchmod +x crc-linux-*/crc mv crc-linux-*/crc "${root_directory}/bin/crc"may expand to two matches and the script aborts. Consider capturing the directory name once:
-curl -L … | tar -xJ - -chmod +x crc-linux-*/crc -mv crc-linux-*/crc "${root_directory}/bin/crc" -rm -rf "crc-linux-*" +tmp_dir=$(mktemp -d) +curl -L … -o "$tmp_dir/crc.tar.xz" +tar -xJf "$tmp_dir/crc.tar.xz" -C "$tmp_dir" +crc_path=$(find "$tmp_dir" -type f -name crc | head -n1) +chmod +x "$crc_path" +delete_if_exists "${root_directory}/bin/crc" +mv "$crc_path" "${root_directory}/bin/crc" +rm -rf "$tmp_dir"Also, consider adding
set -euo pipefailat the top for safer execution.src/roles/clusters/openshift/crc/crc-uninstall/config.yaml (2)
4-7: Clarify description phrasingMinor grammar polish will make the description clearer (“This role helps to uninstall …”).
14-16: Remove trailing whitespace & add terminal newlineyaml-lint is already flagging these. Cleaning them up avoids noisy CI failures.
Also applies to: 31-32
tests/e2e/cluster-lifecycle/crc_install_delete/conftest.py (1)
44-50: Sub-process errors should fail the test loudlyWhen a cleanup command fails you only print a warning; the fixture then silently proceeds, hiding real problems (e.g., leaked VMs). Consider
check=Trueor raising after failed cleanup so CI surfaces the issue.Makefile (1)
45-50: Shell variable quoting – preserve whitespace in NEW_PYTEST_ARGS
makeexpands$(NEW_PYTEST_ARGS)before the shell sees it. If the var is unset the condition becomesif [ -n "" ], which is fine, but an empty value explicitly passed (NEW_PYTEST_ARGS="") will still satisfy-n. Usually that’s harmless, but if you need to distinguish unset vs empty, use${NEW_PYTEST_ARGS+set}. No change required – just flagging the subtlety..github/workflows/e2e-cluster-lifecycle.yml (1)
55-55: Trailing whitespaceYaml-lint flags this; harmless but easy to clean.
src/roles/clusters/openshift/crc/crc-uninstall/main.sh (2)
24-24: Quote the sourced path to avoid word-splitting / globbing-source $root_directory/src/commons/scripts/utils.sh +source "$root_directory/src/commons/scripts/utils.sh"
28-30: Unused variablerole_name– delete or use it
role_nameis parsed but never referenced (SC2034). Remove it or export it for callers.src/roles/clusters/openshift/crc/crc-install/main.sh (3)
24-24: Quote the sourced utils path-source $root_directory/src/commons/scripts/utils.sh +source "$root_directory/src/commons/scripts/utils.sh"
28-30:role_nameparsed but never usedConsider removing or exporting it.
78-81: Pull-secret existence check can be simplified & quoted-if [ ! -f "${PULL_SECRET_PATH}" ] && [ ! -f "$(eval echo ${PULL_SECRET_PATH})" ]; then - die "Pull secret file not found at ${PULL_SECRET_PATH}" -fi +expanded_secret="$(eval echo "${PULL_SECRET_PATH}")" +if [ ! -f "$expanded_secret" ]; then + die "Pull secret file not found at $expanded_secret" +fisrc/roles/clusters/openshift/crc/crc-install/config.yaml (3)
19-21: Trailing spaces break YAML lintingRemove the trailing spaces on line 19 to satisfy
yamllint.- - PULL_SECRET_PATH: Set PULL_SECRET_PATH,if you have a pull secret file(default: ~/pull-secret.txt). + - PULL_SECRET_PATH: Set PULL_SECRET_PATH, if you have a pull secret file (default: ~/pull-secret.txt).
13-17: Documentation vs default mismatch for MEMORYDocstring says default
40000but the actual default is10752. Align the two to avoid confusion.
72-74: Cluster type description copy-paste
openshift cluster type (ROSA)is misleading for a local CRC cluster. Replace with “openshift-local”.tests/e2e/cluster-lifecycle/crc_install_delete/test_crc.py (2)
4-4: Remove unused importtime
timeis never referenced.-import time
8-12: Avoidshell=Truein helper – injection riskSince all commands are constant strings, execute without a shell:
-result = subprocess.run(cmd, shell=True, capture_output=True, text=True, env=env) +result = subprocess.run(cmd, capture_output=True, text=True, env=env, shell=False)Convert callers to pass the command as a list.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (20)
.github/workflows/e2e-cluster-lifecycle.yml(3 hunks).vscode/launch.json(1 hunks)Makefile(2 hunks)config.yaml(0 hunks)hacks/download-cli.sh(1 hunks)pytest.ini(1 hunks)src/cli/logics/component.py(1 hunks)src/commons/python/py_utils.py(11 hunks)src/commons/scripts/utils.sh(1 hunks)src/core/initializer.py(1 hunks)src/roles/clusters/kind/install/config.yaml(1 hunks)src/roles/clusters/kind/install/main.sh(2 hunks)src/roles/clusters/openshift/crc/crc-install/config.yaml(1 hunks)src/roles/clusters/openshift/crc/crc-install/main.sh(1 hunks)src/roles/clusters/openshift/crc/crc-uninstall/config.yaml(1 hunks)src/roles/clusters/openshift/crc/crc-uninstall/main.sh(1 hunks)tests/e2e/cluster-lifecycle/crc_install_delete/conftest.py(1 hunks)tests/e2e/cluster-lifecycle/crc_install_delete/test_crc.py(1 hunks)tests/e2e/cluster-lifecycle/kind_install_delete/test_kind.py(2 hunks)tests/e2e/commons/cluster-tests/test_py_utils_e2e.py(4 hunks)
💤 Files with no reviewable changes (1)
- config.yaml
🧰 Additional context used
🪛 YAMLlint (1.37.1)
src/roles/clusters/kind/install/config.yaml
[error] 64-64: no new line character at the end of file
(new-line-at-end-of-file)
src/roles/clusters/openshift/crc/crc-uninstall/config.yaml
[error] 16-16: trailing spaces
(trailing-spaces)
[error] 31-31: trailing spaces
(trailing-spaces)
[error] 32-32: no new line character at the end of file
(new-line-at-end-of-file)
[error] 32-32: trailing spaces
(trailing-spaces)
.github/workflows/e2e-cluster-lifecycle.yml
[error] 55-55: trailing spaces
(trailing-spaces)
src/roles/clusters/openshift/crc/crc-install/config.yaml
[error] 19-19: trailing spaces
(trailing-spaces)
[warning] 66-66: wrong indentation: expected 2 but found 4
(indentation)
[warning] 83-83: too many blank lines
(1 > 0) (empty-lines)
🪛 Shellcheck (0.10.0)
src/roles/clusters/openshift/crc/crc-uninstall/main.sh
[warning] 29-29: role_name appears unused. Verify use (or export if used externally).
(SC2034)
src/roles/clusters/openshift/crc/crc-install/main.sh
[warning] 29-29: role_name appears unused. Verify use (or export if used externally).
(SC2034)
[warning] 85-85: Quote this to prevent word splitting.
(SC2046)
🪛 Pylint (3.3.7)
tests/e2e/cluster-lifecycle/crc_install_delete/test_crc.py
[refactor] 167-167: Too few public methods (1/2)
(R0903)
🪛 Ruff (0.11.9)
tests/e2e/cluster-lifecycle/crc_install_delete/test_crc.py
4-4: time imported but unused
Remove unused import: time
(F401)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: kind-cluster-lifecycle-test (3.12)
- GitHub Check: cluster-test (3.12)
- GitHub Check: non-cluster-test (3.12)
🔇 Additional comments (9)
src/cli/logics/component.py (1)
464-466: Good defensive fix – explicit string coercionConverting every value to
str()before exporting prevents the accidental
injection of non-string types intoos.environ.
Looks ✅ to me.src/roles/clusters/kind/install/main.sh (1)
34-42: Renamedfatal→diematches utils.sh – no issues
utils.shdefinesdie, so switching to it removes an undefined-function
risk. Implementation looks correct.tests/e2e/cluster-lifecycle/kind_install_delete/test_kind.py (1)
8-9: Ensure the new markerkindis registeredPytest will warn/error on unknown markers unless
pytest.ini(or
conftest.py) adds:markers = kind: KIND-specific cluster lifecycle testsVerify that this was added in the same PR; otherwise, add it to
avoidPytestUnknownMarkWarning.src/core/initializer.py (1)
92-95: Environment variables now properly cast to strings – good catchChanging the assignment to
str(stop_when_failed)brings it in line with the next line and avoids the"TypeError: str expected, not bool"surprises some shells / downstream tools can throw. Nothing else to flag here.hacks/download-cli.sh (1)
173-180: Re-using helper for kind cleanup is 👍Switching to
delete_if_existskeeps behaviour consistent across installers..vscode/launch.json (1)
2-30: VS Code switch to Python debug looks good – ensuredebugpyis availableAnyone using this config must have
debugpyin the venv. Maybe add a dev-requirements entry or mention it in the README.src/commons/python/py_utils.py (1)
115-117: Line-wrap only – logic untouchedLooks good; no functional change introduced.
src/roles/clusters/openshift/crc/crc-uninstall/main.sh (1)
40-55:is_positivecontract is counter-intuitive – request verificationThe script treats “truthy” as the string
"0"(seeis_positive) and logs “DELETE is set” only when the return value is0. This is the opposite of Unix exit-code semantics and is easy to mis-read. Please confirm that:is_positive "1" ➔ 1 is_positive "0" ➔ 0 # treated as trueIf that is not intentional, flip the comparison.
tests/e2e/cluster-lifecycle/crc_install_delete/test_crc.py (1)
22-33:is_cluster_stoppedmay raise on absentjq
get_crc_status_jsonassumesjqexists. If not, the function returnsNoneand later JSON parsing can explode. HandleNoneexplicitly or check forjqbefore tests.
| input_env: | ||
| - name: CLEANUP | ||
| description: | | ||
| Set CLEANUP to true if you want to stop/cleanup crc cluster (default: false) | ||
| default: false | ||
| - name: DELETE | ||
| description: | | ||
| Set DELETE to true if you want to delete crc cluster vm (default: false) | ||
| default: false |
There was a problem hiding this comment.
YAML list indentation is invalid – file will not parse
- name: items must be indented two spaces deeper than the input_env: key. Current layout breaks the mapping and will cause the role loader to explode.
22 input_env:
-23 - name: CLEANUP
-24 description: |
-25 Set CLEANUP to true if you want to stop/cleanup crc cluster (default: false)
-26 default: false
-27 - name: DELETE
-28 description: |
-29 Set DELETE to true if you want to delete crc cluster vm (default: false)
-30 default: false
+23 - name: CLEANUP
+24 description: |
+25 Set CLEANUP to true if you want to stop/cleanup crc cluster (default: false)
+26 default: false
+27 - name: DELETE
+28 description: |
+29 Set DELETE to true if you want to delete crc cluster vm (default: false)
+30 default: false
+
+# Add a newline at EOF too📝 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.
| input_env: | |
| - name: CLEANUP | |
| description: | | |
| Set CLEANUP to true if you want to stop/cleanup crc cluster (default: false) | |
| default: false | |
| - name: DELETE | |
| description: | | |
| Set DELETE to true if you want to delete crc cluster vm (default: false) | |
| default: false | |
| input_env: | |
| - name: CLEANUP | |
| description: | | |
| Set CLEANUP to true if you want to stop/cleanup crc cluster (default: false) | |
| default: false | |
| - name: DELETE | |
| description: | | |
| Set DELETE to true if you want to delete crc cluster vm (default: false) | |
| default: false | |
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-uninstall/config.yaml around lines 22 to
30, the YAML list items under input_env are not indented correctly. Fix the
indentation by adding two spaces before each '- name:' entry so they are
properly nested under the input_env key, ensuring the YAML parses correctly and
the role loader does not fail.
| @pytest.fixture() | ||
| def role_env(base_env): | ||
| """Fixture providing base environment variables needed for all tests""" | ||
| base_env["DELETE"] = "1" # 1 means false in the role | ||
| base_env["CLEANUP"] = "1" # 1 means false in the role | ||
| base_env["KUBEADMIN_PASSWORD"] = "kubeadmin" # Default CRC admin password | ||
| base_env["ADD_CLUSTER_ADMIN_TO_DEVELOPER"] = "1" # 1 means false in the role | ||
| return base_env |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid mutating the shared base_env fixture in-place
base_env is session-scoped; changing it here leaks values between independent tests/classes. Work on a copy instead:
-@pytest.fixture()
-def role_env(base_env):
+@pytest.fixture()
+def role_env(base_env):
- base_env["DELETE"] = "1"
- base_env["CLEANUP"] = "1"
- base_env["KUBEADMIN_PASSWORD"] = "kubeadmin"
- base_env["ADD_CLUSTER_ADMIN_TO_DEVELOPER"] = "1"
- return base_env
+ env = base_env.copy()
+ env.update(
+ {
+ "DELETE": "1",
+ "CLEANUP": "1",
+ "KUBEADMIN_PASSWORD": "kubeadmin",
+ "ADD_CLUSTER_ADMIN_TO_DEVELOPER": "1",
+ }
+ )
+ return env📝 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.
| @pytest.fixture() | |
| def role_env(base_env): | |
| """Fixture providing base environment variables needed for all tests""" | |
| base_env["DELETE"] = "1" # 1 means false in the role | |
| base_env["CLEANUP"] = "1" # 1 means false in the role | |
| base_env["KUBEADMIN_PASSWORD"] = "kubeadmin" # Default CRC admin password | |
| base_env["ADD_CLUSTER_ADMIN_TO_DEVELOPER"] = "1" # 1 means false in the role | |
| return base_env | |
| @pytest.fixture() | |
| def role_env(base_env): | |
| """Fixture providing base environment variables needed for all tests""" | |
| env = base_env.copy() | |
| env.update( | |
| { | |
| "DELETE": "1", | |
| "CLEANUP": "1", | |
| "KUBEADMIN_PASSWORD": "kubeadmin", | |
| "ADD_CLUSTER_ADMIN_TO_DEVELOPER": "1", | |
| } | |
| ) | |
| return env |
🤖 Prompt for AI Agents
In tests/e2e/cluster-lifecycle/crc_install_delete/conftest.py around lines 24 to
31, the role_env fixture mutates the shared session-scoped base_env dictionary
in-place, causing state leakage between tests. To fix this, create a copy of
base_env at the start of the fixture and modify the copy instead of the
original. Return the modified copy to ensure isolation between tests.
| if ! echo "$CHANGED_FILES" | grep -qE '^(src/roles/clusters/kind/|tests/e2e/cluster-life-cycle/kind_install_delete/|default_provided_services/|hacks/|.github/|commons/)'; then | ||
| echo "No relevant files changed for KIND cluster-lifecycle-test. Skipping tests but marking success." | ||
| exit 0 |
There was a problem hiding this comment.
Directory glob has a typo – tests will be skipped unintentionally
cluster-life-cycle ≠ cluster-lifecycle. The current regex never matches the actual paths, so the workflow always short-circuits.
- if ! echo "$CHANGED_FILES" | grep -qE '^(src/roles/clusters/kind/|tests/e2e/cluster-life-cycle/kind_install_delete/|default_provided_services/|hacks/|.github/|commons/)'; then
+ if ! echo "$CHANGED_FILES" | grep -qE '^(src/roles/clusters/kind/|tests/e2e/cluster-lifecycle/kind_install_delete/|default_provided_services/|hacks/|.github/|commons/)'; thenMake the same correction in the commented-out OpenShift block below.
📝 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.
| if ! echo "$CHANGED_FILES" | grep -qE '^(src/roles/clusters/kind/|tests/e2e/cluster-life-cycle/kind_install_delete/|default_provided_services/|hacks/|.github/|commons/)'; then | |
| echo "No relevant files changed for KIND cluster-lifecycle-test. Skipping tests but marking success." | |
| exit 0 | |
| if ! echo "$CHANGED_FILES" | grep -qE '^(src/roles/clusters/kind/|tests/e2e/cluster-lifecycle/kind_install_delete/|default_provided_services/|hacks/|.github/|commons/)'; then | |
| echo "No relevant files changed for KIND cluster-lifecycle-test. Skipping tests but marking success." | |
| exit 0 |
🤖 Prompt for AI Agents
In .github/workflows/e2e-cluster-lifecycle.yml around lines 24 to 26, the
directory glob pattern 'tests/e2e/cluster-life-cycle/kind_install_delete/'
contains a typo with hyphens instead of the correct 'cluster-lifecycle'. Update
the regex to use 'cluster-lifecycle' to correctly match the intended paths and
prevent unintended skipping of tests. Also, locate the commented-out OpenShift
block in the same file and apply the same correction to its directory glob
pattern.
| crc_status=$(crc status 2>&1) | ||
| if echo "$crc_status" | grep -iq "Machine does not exist\|crc does not seem to be setup correctly" || [ "$(crc status -o json | jq -r .openshiftStatus)" = "Stopped" ]; then | ||
| success "OpenShift local cluster is properly stopped" | ||
| else | ||
| fail "OpenShift local cluster is not properly stopped" | ||
| result=1 | ||
| fi |
There was a problem hiding this comment.
Hard dependency on jq is unchecked
crc status -o json | jq … will fail with “command not found” if jq is absent and will make the script report a running cluster as failed. Add a pre-flight check:
command -v jq >/dev/null || die "jq is required but not installed"🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-uninstall/main.sh around lines 70 to 76,
the script uses jq without verifying if it is installed, which can cause
failures if jq is missing. Add a pre-flight check before this block using
'command -v jq >/dev/null || die "jq is required but not installed"' to ensure
jq is available and exit with a clear error message if it is not.
| # Traverse up the directory tree to find the .github folder | ||
| github_dir="$current_dir" | ||
| while [ ! -d "$github_dir/.git" ] && [ "$github_dir" != "/" ]; do | ||
| github_dir="$(dirname "$github_dir")" | ||
| done | ||
|
|
||
| # If the .github folder is found, set root_directory | ||
| if [ -d "$github_dir/.git" ]; then | ||
| root_directory="$github_dir" | ||
| echo "The root directory is: $root_directory" | ||
| else | ||
| echo "Unable to find .github folder" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Misleading comment & missing guard when repository root is not found
The comment talks about searching for a “.github” folder while the loop actually looks for .git.
If a .git directory is never found, root_directory is left unset, yet the script continues and source $root_directory/... will expand to an empty path and silently succeed or fail later.
-# Traverse up the directory tree to find the .github folder
+# Traverse up the directory tree to find the repository root (.git)
...
-else
- echo "Unable to find .github folder"
-fi
+# Abort early when repository root is not located
+else
+ die "Unable to locate repository root (.git); aborting"
+fi📝 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.
| # Traverse up the directory tree to find the .github folder | |
| github_dir="$current_dir" | |
| while [ ! -d "$github_dir/.git" ] && [ "$github_dir" != "/" ]; do | |
| github_dir="$(dirname "$github_dir")" | |
| done | |
| # If the .github folder is found, set root_directory | |
| if [ -d "$github_dir/.git" ]; then | |
| root_directory="$github_dir" | |
| echo "The root directory is: $root_directory" | |
| else | |
| echo "Unable to find .github folder" | |
| fi | |
| # Traverse up the directory tree to find the repository root (.git) | |
| github_dir="$current_dir" | |
| while [ ! -d "$github_dir/.git" ] && [ "$github_dir" != "/" ]; do | |
| github_dir="$(dirname "$github_dir")" | |
| done | |
| # If the .github folder is found, set root_directory | |
| if [ -d "$github_dir/.git" ]; then | |
| root_directory="$github_dir" | |
| echo "The root directory is: $root_directory" | |
| # Abort early when repository root is not located | |
| else | |
| die "Unable to locate repository root (.git); aborting" | |
| fi |
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-uninstall/main.sh around lines 10 to 22,
update the comment to correctly state that the script is searching for the
".git" directory, not ".github". Add a guard after the search loop to check if
the repository root was found by verifying if root_directory is set; if not,
exit the script with an error message to prevent sourcing from an empty or
invalid path.
| # Verify openshift local cluster is running | ||
| if ! retry 60 5 "crc status -o json|jq -r .openshiftStatus | grep -q Running" "Waiting for openshift local cluster to be ready"; then | ||
| fail "Openshift local cluster is not running properly after 5 minutes" | ||
| result=1 | ||
| else | ||
| success "Openshift local cluster is running properly" | ||
| fi |
There was a problem hiding this comment.
Unverified dependencies: jq, oc, retry
The verify loop depends on these tools/functions but the script only checks for crc. Add explicit checks:
for bin in jq oc; do
command -v "$bin" >/dev/null || die "$bin not installed"
done
type retry >/dev/null 2>&1 || die "retry helper not sourced"🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-install/main.sh around lines 97 to 103,
the script uses jq, oc, and the retry function without verifying their
availability. Add explicit checks before the verify loop to ensure jq and oc
binaries are installed using command -v, and confirm the retry function is
sourced by checking its type. If any are missing, terminate the script with a
clear error message.
| # Traverse up the directory tree to find the .github folder | ||
| github_dir="$current_dir" | ||
| while [ ! -d "$github_dir/.git" ] && [ "$github_dir" != "/" ]; do | ||
| github_dir="$(dirname "$github_dir")" | ||
| done | ||
|
|
||
| # If the .github folder is found, set root_directory | ||
| if [ -d "$github_dir/.git" ]; then | ||
| root_directory="$github_dir" | ||
| echo "The root directory is: $root_directory" | ||
| else | ||
| echo "Unable to find .github folder" | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Same root-directory search issues as uninstall script
Replicate the earlier fix: correct the comment and abort when .git is not found to prevent undefined $root_directory.
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-install/main.sh around lines 10 to 22,
update the comment to correctly describe searching for the .git folder instead
of .github. Add a check after the search loop to abort the script with an error
message if the .git folder is not found, preventing the use of an undefined
root_directory variable.
| if [[ $DEBUG == "0" ]]; then | ||
| set -x | ||
| fi |
There was a problem hiding this comment.
🛠️ Refactor suggestion
set -x leaks secrets when DEBUG==0
Because DEBUG==0 enables tracing, sensitive variables such as KUBEADMIN_PASSWORD and tokens are echoed to the log. Reverse the check or guard secret exports.
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-install/main.sh around lines 3 to 5, the
current logic enables shell tracing with 'set -x' when DEBUG is 0, which causes
sensitive information like passwords and tokens to be printed in logs. To fix
this, reverse the condition so that 'set -x' is only enabled when DEBUG is not
0, or alternatively, add safeguards to prevent secret variables from being
echoed when tracing is enabled.
| crc config set memory ${MEMORY} >/dev/null 2>&1 | ||
| crc config set cpus ${CPU} >/dev/null 2>&1 | ||
| crc config set disk-size ${DISK_SIZE} >/dev/null 2>&1 | ||
| crc config set kubeadmin-password ${KUBEADMIN_PASSWORD} >/dev/null 2>&1 | ||
| crc config set enable-cluster-monitoring ${ENABLE_MONITORING} >/dev/null 2>&1 | ||
| else |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Unquoted variable expansions risk word-splitting
crc config set ... ${MEMORY} etc. are safe, but the pull-secret path is not:
-crc start -p $(eval echo ${PULL_SECRET_PATH})
+crc start -p "$(eval echo "${PULL_SECRET_PATH}")"Also quote the earlier crc config set values for consistency.
Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-install/main.sh around lines 69 to 74,
the variable expansions in the crc config set commands are unquoted, risking
word-splitting and errors especially for the pull-secret path. To fix this, add
double quotes around all variable expansions like "${MEMORY}", "${CPU}",
"${DISK_SIZE}", "${KUBEADMIN_PASSWORD}", and "${ENABLE_MONITORING}" to ensure
safe and consistent handling of values.
| - name: CLUSTER_CONSOLE_URL | ||
| description: openshift console url | ||
|
|
||
| - name: CLUSTER_API_URL | ||
| description: openshift api url | ||
|
|
||
| - name: CLUSTER_TYPE | ||
| description: openshift cluster type (ROSA) | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Indentation off by two spaces – fails strict validators
output_env block is indented by four spaces where two are expected. Fix indentation to avoid tooling issues.
🧰 Tools
🪛 YAMLlint (1.37.1)
[warning] 66-66: wrong indentation: expected 2 but found 4
(indentation)
🤖 Prompt for AI Agents
In src/roles/clusters/openshift/crc/crc-install/config.yaml around lines 66 to
74, the entries under the output_env block are indented by four spaces instead
of the required two spaces. Adjust the indentation of these lines to two spaces
to comply with strict YAML validators and avoid tooling issues.
|
/lgtm |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
1 participant
Summary by CodeRabbit
New Features
Enhancements
Bug Fixes
Style
Chores