Fix | Remove reference to Terraform in error message #312
Conversation
Pull Request Test Coverage Report for Build 17227204968Details
💛 - Coveralls |
WalkthroughMinor textual and configuration updates: typo and message wording fixes, README example variable rename (TERRAFORM_IMAGE_TAG → TF_IMAGE_TAG), CI/workflow and Python-version bumps (adds 3.13, GitHub Actions updates), dependency/package metadata updates, and small docstring/comment edits. No public API signature changes. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20–30 minutes Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ 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). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
* Upgrade python-hcl2 library * Update lock file * Fix integration test workflows * Drop tofu image for now
![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
🔭 Outside diff range comments (1)
leverage/path.py (1)
1-1: Commit Black formatting for leverage/path.pyThe file has now been reformatted by Black. Please commit the updated
leverage/path.pyso that CI passes its Black check.
🧹 Nitpick comments (2)
leverage/modules/credentials.py (1)
160-160: Typo fix LGTM; optional copy polishThe correction to “programmatic” is right. Optionally consider aligning with AWS UI wording for clarity.
Apply if you prefer:
- "message": "Select the means by which you'll provide the programmatic keys:", + "message": "Select how you'll provide AWS access keys:",leverage/path.py (1)
281-285: Message matches PR intent; fix grammar for clarityRemoving “Terraform” aligns with the PR goal. The phrase “cannot run neither … or …” is ungrammatical. Suggest a concise alternative.
Apply this diff:
- "This command cannot run neither in the root of the project or in" - " the root directory of an account.", + "This command cannot run in the project's root or in an account's root directory.",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
leverage/modules/credentials.py(1 hunks)leverage/path.py(1 hunks)
🧰 Additional context used
🪛 GitHub Actions: Tests | Lint w/ Black
leverage/path.py
[error] 1-1: Black formatting check failed (black --check --verbose). 1 file would be reformatted: leverage/path.py. Run 'black leverage/path.py' to fix formatting.
* Support python 3.13 * Make workflows use a newer python version * Update lockfile * Make it all 3.13 * Accept CodeRabbit suggestions * Fix converalls run * Small improvement in README * Fix lock file * Explicitly call terraform command
…verage into fix/confusing-error-message
coderabbitai
bot
requested review from
Franr,
diego-ojeda-binbash and
exequielrafaela
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
.github/workflows/tests-unit.yml (2)
26-31: Fix env var export and Poetry PATH; current commands are ineffective.
export ENV POETRY_VIRTUALENVS_CREATE=falsesets a variable named ENV, not POETRY_VIRTUALENVS_CREATE.- Poetry 1.8 installs to ~/.local/bin by default; adding ~/.poetry/bin is a no-op on fresh runners.
- name: Install dependencies using Poetry run: | echo "[INFO] Installing dependencies..." - export ENV POETRY_VIRTUALENVS_CREATE=false - export PATH="${PATH}:${HOME}/.poetry/bin" + export POETRY_VIRTUALENVS_CREATE=false + export PATH="${PATH}:${HOME}/.local/bin" poetry install --with=dev
33-38: Use correct Poetry PATH during test execution.
Ensure the Poetry shim is resolvable on a clean runner.- name: Run unit tests run: | echo "[INFO] Running unit tests and generate coverage report" - export PATH="${PATH}:${HOME}/.poetry/bin" + export PATH="${PATH}:${HOME}/.local/bin" poetry run pytest --verbose --cov=./leverage/ --cov-report=xml.github/workflows/release-pypi-build-push-package.yml (1)
29-31: Fix env export and Poetry PATH here as well.
Same issues as unit tests workflow.- name: Install dependencies using Poetry run: | echo "[INFO] Installing dependencies..." - export ENV POETRY_VIRTUALENVS_CREATE=false - export PATH="${PATH}:${HOME}/.poetry/bin" + export POETRY_VIRTUALENVS_CREATE=false + export PATH="${PATH}:${HOME}/.local/bin" poetry install --with=dev.github/workflows/release-pypi-build-push-test-package.yml (1)
33-35: Correct env export and Poetry PATH.
Mirror the fix applied in other workflows.- name: Install dependencies using Poetry run: | echo "[INFO] Installing dependencies..." - export ENV POETRY_VIRTUALENVS_CREATE=false - export PATH="${PATH}:${HOME}/.poetry/bin" + export POETRY_VIRTUALENVS_CREATE=false + export PATH="${PATH}:${HOME}/.local/bin" poetry install --with=dev.github/workflows/tests-integration.yaml (2)
34-37: Upgrade actions/setup-python to v5 for Python 3.13 support and improved caching.
@v2is outdated and may not fully support 3.13 nuances. Use the maintained major.- - name: Set up Python ${{ matrix.python-version }} - uses: actions/setup-python@v2 + - name: Set up Python ${{ matrix.python-version }} + uses: actions/setup-python@v5 with: python-version: ${{ matrix.python-version }}
47-51: Environment var export typo effectively no-ops Poetry config.
export ENV POETRY_VIRTUALENVS_CREATE=falsedefines a variable namedENV, notPOETRY_VIRTUALENVS_CREATE. Either drop it (you already runpoetry config virtualenvs.create false) or fix the export.- export ENV POETRY_VIRTUALENVS_CREATE=false + # already configured via 'poetry config virtualenvs.create false' + # If you prefer env-based control, use: + # export POETRY_VIRTUALENVS_CREATE=false
🧹 Nitpick comments (10)
.github/workflows/tests-unit.yml (2)
12-17: Upgrade Actions to current majors for consistency and security.
Other workflows in this PR already use checkout@v4 and setup-python@v5. Align this file too.- - uses: actions/checkout@v3 + - uses: actions/checkout@v4 @@ - - name: Set up Python - uses: actions/setup-python@v2 + - name: Set up Python + uses: actions/setup-python@v5
19-25: Optional: add dependency caching to speed up CI.
Caching Poetry’s download and virtualenv directories typically saves minutes per run.Example step you can insert after “Set up Python”:
- name: Cache Poetry and virtualenvs uses: actions/cache@v4 with: path: | ~/.cache/pypoetry ~/.cache/pip ./.venv key: poetry-${{ runner.os }}-${{ matrix.python-version }}-${{ hashFiles('**/poetry.lock') }} restore-keys: | poetry-${{ runner.os }}-${{ matrix.python-version }}-Also applies to: 26-31
pyproject.toml (1)
30-30: Simplify Python version constraint.
The OR of tilde ranges works but is verbose. Consider a single bounded range for readability and fewer merge conflicts.-[tool.poetry.dependencies] -python = "~3.9 || ~3.10 || ~3.11 || ~3.12 || ~3.13" +[tool.poetry.dependencies] +python = ">=3.9,<3.14"leverage/modules/project.py (1)
202-209: Avoid mutating the input config inside the render loop.The defaulting for
tf_image_tagruns inside the per-template loop and mutates the caller’s dict. It’s harmless but surprising. Set defaults once before the loop using a shallow copy.def _render_templates(template_files, config, source=TEMPLATE_DIR, destination=PROJECT_ROOT): @@ - for template_file in template_files: + # Work on a copy and set defaults once + cfg = dict(config) + if "tf_image_tag" not in cfg: + cfg["tf_image_tag"] = cfg.get("terraform_image_tag", __toolbox_version__) + + for template_file in template_files: @@ - rendered_template = template.render(config) + rendered_template = template.render(cfg)README.md (2)
53-54: Example env var rename looks good; consider aligning the example tag with CI matrix.The
TF_IMAGE_TAG=1.5.0-0.2.0example is fine. Since CI now tests...-0.2.3, you might optionally bump the example tag to a currently tested one to reduce confusion for readers skimming both docs and workflows.Would you like me to verify on Docker Hub which of
1.3.5-0.2.3,1.5.0-0.2.3, or1.6.0-0.2.3are available and suggest one as the canonical example?
115-116: Minor grammar fix in the explanatory sentence.Tweak phrasing for concision and correctness.
-This setup commands `pyenv` to use `leverage_py_313_venv` as the local Python version for your project directory, -ensuring that all Python operations within this directory use this isolated environment. +These commands instruct pyenv to use `leverage_py_313_venv` as the local Python version for your project directory, +ensuring all Python operations within this directory use this isolated environment.Also applies to: 120-121, 123-124, 132-136
.github/workflows/tests-integration.yaml (4)
28-29: Matrix looks good; verify toolbox tags exist before pushing.The expanded Python matrix (including 3.13) and bumped toolbox tags are sensible. To avoid red builds when a tag is missing, consider validating the tag exists before proceeding.
Optional preflight step:
matrix: python-version: ['3.9', '3.10', '3.11', '3.12', '3.13'] toolbox-image-tag: ['1.3.5-0.2.3', '1.5.0-0.2.3', '1.6.0-0.2.3'] + env: + TOOLBOX_TAG: ${{ matrix.toolbox-image-tag }} @@ steps: + - name: Verify toolbox tag exists + run: | + set -euo pipefail + echo "[INFO] Verifying toolbox tag ${TOOLBOX_TAG} exists" + curl -fsSL "https://hub.docker.com/v2/repositories/binbash/leverage-toolbox/tags/${TOOLBOX_TAG}" >/dev/null + echo "[INFO] Tag found"
31-33: Follow actions/checkout latest major.Move to
actions/checkout@v4(performance and security updates).- - name: Checkout base branch - uses: actions/checkout@v3 + - name: Checkout base branch + uses: actions/checkout@v4
60-66: Use mkdir -p for idempotency.
mkdir ~/.sshwill fail if the directory already exists on the runner;-pis safer.- mkdir ~/.ssh + mkdir -p ~/.ssh
219-221: Nit: message wording.“Destroying all generated created resources” → “Destroying all created resources”.
- printf "[INFO] Destroying all generated created resources\n" + printf "[INFO] Destroying all created resources\n"
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
⛔ Files ignored due to path filters (1)
poetry.lockis excluded by!**/*.lock
📒 Files selected for processing (11)
.github/release-drafter.yml(1 hunks).github/workflows/release-pypi-build-push-package.yml(1 hunks).github/workflows/release-pypi-build-push-test-package.yml(1 hunks).github/workflows/tests-integration.yaml(3 hunks).github/workflows/tests-unit.yml(2 hunks)README.md(4 hunks)leverage/container.py(3 hunks)leverage/modules/credentials.py(2 hunks)leverage/modules/project.py(2 hunks)leverage/path.py(1 hunks)pyproject.toml(2 hunks)
✅ Files skipped from review due to trivial changes (1)
- leverage/container.py
🚧 Files skipped from review as they are similar to previous changes (2)
- leverage/modules/credentials.py
- leverage/path.py
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-10-17T16:31:27.570Z
Learnt from: exequielrafaela
PR: binbashar/leverage#289
File: .github/workflows/tests-integration.yaml:28-29
Timestamp: 2024-10-17T16:31:27.570Z
Learning: The toolbox image tags `1.3.5-0.2.0`, `1.5.0-0.2.0`, and `1.6.0-0.2.0` exist in the registry.
Applied to files:
README.md
🧬 Code graph analysis (2)
leverage/modules/project.py (1)
leverage/logger.py (1)
info(116-118)
README.md (1)
leverage/modules/tf.py (1)
terraform(46-57)
🪛 Ruff (0.12.2)
leverage/modules/project.py
206-206: Test for membership should be not in
Convert to not in
(E713)
🪛 LanguageTool
README.md
[grammar] ~135-~135: There might be a mistake here.
Context: ...thon version for your project directory, ensuring that all Python operations with...
(QB_NEW_EN)
🪛 YAMLlint (1.37.1)
.github/release-drafter.yml
[error] 39-39: trailing spaces
(trailing-spaces)
🔇 Additional comments (13)
.github/workflows/tests-unit.yml (2)
10-10: Matrix: good call adding Python 3.13.
Including 3.13 in CI helps surface compat issues early across the supported range.
40-45: Coveralls on a single matrix entry looks good.
Limiting upload to 3.13 avoids duplicate reports..github/release-drafter.yml (1)
41-45: Template: $CHANGES placeholder addition is correct.
Release Drafter will render the accumulated change lines here.pyproject.toml (2)
20-21: Classifiers updated for 3.12/3.13 — looks good.
Matches the CI matrix and signals runtime support accurately.
38-38: Manual Python 3.13 compatibility check neededThe automated script didn’t complete successfully in this environment because:
- The default
pythonis 3.11.2, not 3.13- The newly installed
poetrybinary isn’t on$PATHpytestisn’t availablePlease verify that all pinned dependencies—including
click 8.0.1,jinja2 3.0.1,requests 2.31,rich 10.4.0, andpython-hcl2 7.3.1—install and test cleanly under Python 3.13. You can use the existing CI matrix or run locally, for example:# Ensure Python 3.13 and Poetry are installed and on your PATH python3.13 --version pip install poetry==1.8.2 --user export PATH="$HOME/.local/bin:$PATH" # Switch Poetry to use Python 3.13 poetry env use python3.13 # Re-lock (no updates), install all deps, and run tests poetry lock --no-update poetry install --with dev pytest -q.github/workflows/release-pypi-build-push-package.yml (2)
11-11: Checkout upgraded to v4 — good.
Stays current with upstream deprecations.
15-17: Setup-python bumped to v5 and Python 3.13 — good alignment.
Matches supported runtimes in pyproject and test matrix..github/workflows/release-pypi-build-push-test-package.yml (2)
17-17: Checkout upgraded to v4 — LGTM.
19-21: Setup-python v5 with Python 3.13 — LGTM.leverage/modules/project.py (1)
327-327: LGTM: neutralized wording.“Reformatting configuration to the standard style.” reads clearer and avoids tool-specific wording. No action needed.
README.md (2)
59-60: Migration note reads clearly.The instruction to migrate
TF_IMAGE_TAGfrom9.9.9to9.9.9-9.9.9is concise and actionable. Nice.
73-74: Adding Python 3.13 support: double-check packaging metadata and constraints.Since 3.13 is advertised here, ensure:
pyproject.tomlclassifiers includeProgramming Language :: Python :: 3.13.- Dependency pins are compatible with 3.13 (Poetry lock or constraints).
- CI unit and integration matrices both run on 3.13 (looks covered elsewhere).
I can scan the repo and open a follow-up if anything’s missing.
.github/workflows/tests-integration.yaml (1)
144-146: LGTM: CLI alias usage clarified.Switching to
leverage terraform init --skip-validationaligns the workflow with the CLI’s canonical subcommand. No action needed.
| printf "[INFO] Initializing layer\n" | ||
| leverage tf init --layers cli-test-layer,base-identities | ||
| leverage terraform init --layers cli-test-layer,base-identities | ||
| printf "[INFO] Generating plan\n" | ||
| leverage tf plan --layers cli-test-layer | ||
| leverage terraform plan --layers cli-test-layer | ||
| printf "[INFO] Applying changes\n" | ||
| leverage tf apply -auto-approve --layers cli-test-layer | ||
| leverage terraform apply -auto-approve --layers cli-test-layer | ||
| printf "[INFO] Checking if all changes were applied\n" | ||
| leverage tf plan -detailed-exitcode --layers cli-test-layer | ||
| leverage terraform plan -detailed-exitcode --layers cli-test-layer | ||
| [[ $? -eq 2 ]] && printf "[WARN] There are still remaining changes\n" | ||
| [[ $? -eq 0 ]] && printf "[INFO] Apply checks out\n" |
There was a problem hiding this comment.
Bug: exit code checks are incorrect due to clobbered $?.
After plan -detailed-exitcode, $? is read once, then the next [[ ... ]] overwrites it, so the second check no longer inspects the plan’s exit code. Capture it once and branch.
- leverage terraform plan -detailed-exitcode --layers cli-test-layer
- [[ $? -eq 2 ]] && printf "[WARN] There are still remaining changes\n"
- [[ $? -eq 0 ]] && printf "[INFO] Apply checks out\n"
+ leverage terraform plan -detailed-exitcode --layers cli-test-layer
+ plan_rc=$?
+ if [[ $plan_rc -eq 2 ]]; then
+ printf "[WARN] There are still remaining changes\n"
+ elif [[ $plan_rc -eq 0 ]]; then
+ printf "[INFO] Apply checks out\n"
+ else
+ printf "[ERROR] Unexpected terraform plan exit code: %d\n" "$plan_rc"
+ exit $plan_rc
+ 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.
| printf "[INFO] Initializing layer\n" | |
| leverage tf init --layers cli-test-layer,base-identities | |
| leverage terraform init --layers cli-test-layer,base-identities | |
| printf "[INFO] Generating plan\n" | |
| leverage tf plan --layers cli-test-layer | |
| leverage terraform plan --layers cli-test-layer | |
| printf "[INFO] Applying changes\n" | |
| leverage tf apply -auto-approve --layers cli-test-layer | |
| leverage terraform apply -auto-approve --layers cli-test-layer | |
| printf "[INFO] Checking if all changes were applied\n" | |
| leverage tf plan -detailed-exitcode --layers cli-test-layer | |
| leverage terraform plan -detailed-exitcode --layers cli-test-layer | |
| [[ $? -eq 2 ]] && printf "[WARN] There are still remaining changes\n" | |
| [[ $? -eq 0 ]] && printf "[INFO] Apply checks out\n" | |
| printf "[INFO] Checking if all changes were applied\n" | |
| leverage terraform plan -detailed-exitcode --layers cli-test-layer | |
| plan_rc=$? | |
| if [[ $plan_rc -eq 2 ]]; then | |
| printf "[WARN] There are still remaining changes\n" | |
| elif [[ $plan_rc -eq 0 ]]; then | |
| printf "[INFO] Apply checks out\n" | |
| else | |
| printf "[ERROR] Unexpected terraform plan exit code: %d\n" "$plan_rc" | |
| exit $plan_rc | |
| fi |
🤖 Prompt for AI Agents
.github/workflows/tests-integration.yaml around lines 205 to 217: the script
runs `leverage terraform plan -detailed-exitcode` then checks `$?` twice, but
the first `[[ ... ]]` test overwrites `$?` so the second check inspects the test
command's exit status instead of the plan's; capture the plan exit code
immediately into a variable (e.g., plan_exit=$?) and then use that variable for
the conditional branches so both checks examine the original plan result,
leaving the rest of the messages intact.
What?
Summary by CodeRabbit
Bug Fixes
Documentation
Chores / CI
Packaging