Repository navigation
Feat | Remove docker dependency - #316
Merged
Merged
Conversation
angelofenoglio
requested review from
Franr,
diego-ojeda-binbash,
exequielrafaela and
juanmatias
October 24, 2025 14:45
Contributor
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the
✨ 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 |
angelofenoglio
force-pushed
the
feat/remove-docker-dependency
branch
from
October 24, 2025 14:55
8d1412a to
11917b7
Compare
angelofenoglio
marked this pull request as ready for review
March 3, 2026 19:04
exequielrafaela
approved these changes
Mar 26, 2026
exequielrafaela
approved these changes
Apr 9, 2026
This was referenced Aug 30, 2026
diego-ojeda-binbash
added a commit
that referenced
this pull request
Sep 15, 2026
* Fix | Restore the integration tests Fix `leverage project create`: The root command builds the project paths for every invocation, which requires an already configured project. `project init` creates the git repository, so from that point on the configuration loads fine but still holds no project name, and `project create` aborts with "Project name has not been set" on what is its normal, intended use. The command that creates a project could not run on a project that did not exist yet. Before #316 the root command only loaded the config to check the toolbox version, and returned early when it was absent, with a comment noting that the config does not exist yet at some points of the project. That guard was lost when the paths were introduced. The project commands now skip the paths setup, which they never use. A unit test covers the regression. Run the bats tests on the runner, and drop the testing image: The image existed to run the cli inside a container. Since #316 the cli runs the binaries on the host, and the image was never given terraform or tofu, so `leverage terraform version` had been failing on every run since December 2025. What was left of the image was a Linux box with python, bats and leverage in it, which is what the runner already is, and which the unit test workflow already relies on. Dropping it also removes a dind base that no longer had a purpose, the `--privileged` flag and the dockerd entrypoint it needed, and a bind mount that had no effect since the image ran out of the copy made at build time. The no-git test used to `apk del git`, tying it to the image and to the file ordering of the suite. It now runs the cli with a PATH holding nothing but its own entry point, which `shutil.which` resolves the same way, without touching the system. Two portability fixes let the suite run on macOS as well: an explicit mktemp template, since `-t` yields a name with a dot that breaks the module name the build script is imported under, and POSIX classes instead of `\s`, which only glibc accepts. Drop TERRAFORM_IMAGE_TAG from the bats fixtures, unread since #316. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix | Do not require a terminal to run the bats tests The suite ran under `docker run -t`, which allocated a TTY, so it could take one for granted. On the runner there is none, and `$TERM` is unset: tput: No value for $TERM and no -T specified validator.bash: line 8: printf: write error: Broken pipe Two things assumed a terminal. `setup_file` printed a header through `tput`, which fails outright without `$TERM`, and the make target forced the pretty formatter, which needs one. `-t` and `-p` were both passed, which is contradictory since either sets the formatter, and the pretty one won. The header no longer goes through `tput`, and no formatter is forced, so bats picks the pretty one on a terminal and tap when there is none. Verified both ways, with `$TERM` set and unset: 10 tests pass, `make test-int` exits 0, and no tput or broken pipe errors in either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
diego-ojeda-binbash
added a commit
that referenced
this pull request
Sep 15, 2026
* Fix | Restore the unit test suite The unit test workflow has been failing on master since #316, across three consecutive merges. This restores it on the whole 3.9 - 3.13 matrix. Reconcile test_credentials.py with the dockerless design: #316 rewrote leverage/modules/credentials.py but left its tests untouched, so the module failed to import and pytest aborted during collection, which masked every other failure in the run. The tests now inject the runner, paths and config through the click context, as `pass_runner`, `pass_paths` and `pass_state` expect, and account for `Runner.exec` returning a (exit code, stdout, stderr) triple and for the `-mfa` profile suffix that `refresh_layer_credentials_mfa` looks up. Fix _update_account_ids corrupting common.tfvars: #316 replaced the hcledit call with a non-greedy regex that stops at the first closing brace. On a nested `accounts` block, which is what the reference architecture ships, it replaced only the first account, left the remaining ones orphaned outside the block and produced invalid HCL with unbalanced braces. Replacement now scans for the matching brace, and is anchored so `external_accounts` is no longer a candidate match. Fix mock target resolution on Python 3.9: `leverage/modules/__init__.py` re-exports the click Groups, so `leverage.modules.<name>` resolves to the Group rather than to the module. From 3.10 on mock resolves these targets through `pkgutil.resolve_name` and finds the module anyway, but on 3.9 it walks attributes and finds the Group, raising AttributeError during setup. The affected targets are now patched by object, which behaves the same on every supported version. Update test_tf.py for the tfvars injected into init, added deliberately in #316 for ref-arch v2. The tfvars are discovered by globbing, so their order depends on the filesystem and is asserted as a set. Use a raw string for the regex in test_path.py, silencing a SyntaxWarning that becomes a SyntaxError in a future Python version. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix | Make the test suite independent of installed binaries The init tests reached TFRunner.run through the real constructor, so they needed tofu installed to get past binary discovery. Without it the command exited before run() was ever called and the assertions failed on an empty call list, which is what happens on the CI runners. test_discover had the same dependency on kubectl, which happens to be present on GitHub runners and so had been passing by luck. Binary discovery is skipped in both, since the execution itself is mocked. It keeps its own coverage in test_runner.py and test_tfrunner.py. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix | Restore the integration tests (#323) * Fix | Restore the integration tests Fix `leverage project create`: The root command builds the project paths for every invocation, which requires an already configured project. `project init` creates the git repository, so from that point on the configuration loads fine but still holds no project name, and `project create` aborts with "Project name has not been set" on what is its normal, intended use. The command that creates a project could not run on a project that did not exist yet. Before #316 the root command only loaded the config to check the toolbox version, and returned early when it was absent, with a comment noting that the config does not exist yet at some points of the project. That guard was lost when the paths were introduced. The project commands now skip the paths setup, which they never use. A unit test covers the regression. Run the bats tests on the runner, and drop the testing image: The image existed to run the cli inside a container. Since #316 the cli runs the binaries on the host, and the image was never given terraform or tofu, so `leverage terraform version` had been failing on every run since December 2025. What was left of the image was a Linux box with python, bats and leverage in it, which is what the runner already is, and which the unit test workflow already relies on. Dropping it also removes a dind base that no longer had a purpose, the `--privileged` flag and the dockerd entrypoint it needed, and a bind mount that had no effect since the image ran out of the copy made at build time. The no-git test used to `apk del git`, tying it to the image and to the file ordering of the suite. It now runs the cli with a PATH holding nothing but its own entry point, which `shutil.which` resolves the same way, without touching the system. Two portability fixes let the suite run on macOS as well: an explicit mktemp template, since `-t` yields a name with a dot that breaks the module name the build script is imported under, and POSIX classes instead of `\s`, which only glibc accepts. Drop TERRAFORM_IMAGE_TAG from the bats fixtures, unread since #316. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * Fix | Do not require a terminal to run the bats tests The suite ran under `docker run -t`, which allocated a TTY, so it could take one for granted. On the runner there is none, and `$TERM` is unset: tput: No value for $TERM and no -T specified validator.bash: line 8: printf: write error: Broken pipe Two things assumed a terminal. `setup_file` printed a header through `tput`, which fails outright without `$TERM`, and the make target forced the pretty formatter, which needs one. `-t` and `-p` were both passed, which is contradictory since either sets the formatter, and the pretty one won. The header no longer goes through `tput`, and no formatter is forced, so bats picks the pretty one on a terminal and tap when there is none. Verified both ways, with `$TERM` set and unset: 10 tests pass, `make test-int` exits 0, and no tput or broken pipe errors in either. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
juanmatias
added a commit
that referenced
this pull request
Sep 17, 2026
`leverage credentials configure --type BOOTSTRAP` crashed with "Command execution failed: The config profile (...) could not be found" on a first-ever run, because `_profile_is_configured` calls `Runner.exec(...)` without `raises=False`. `Runner.exec` defaults to raising an ExitError on any nonzero exit code, so the intended "probe, then report" logic (`return not exit_code`) never runs when the profile has never been configured (exit 255) - it aborts the whole command instead. This is a regression from the Docker-removal refactor (#316), which replaced the old non-raising `_exec` with the new `Runner`/`Runner.exec` (raises by default) without updating call sites written for the old contract. Six more `awscli.exec(...)` calls in credentials.py had the identical shape; `_credentials_are_valid` and `_get_organization_accounts` had real dead graceful-fallback logic, while `_get_management_account_id`, `_get_mfa_serial`, and the two `configure`/`configure_profile` key-setting loops only lost their more specific custom error message to Runner's generic one. All seven now explicitly pass `raises=False`. Hardened the `awscli_returning` test double to mimic Runner.exec's real raising contract, so a future regression of this shape fails its own test instead of passing silently, and added direct coverage for `_profile_is_configured` (previously untested). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
angelofenoglio
pushed a commit
that referenced
this pull request
Oct 15, 2026
* Fix | Skip PathsHandler for credentials configure during project bootstrap leverage credentials configure crashed with "Project name has not been set" when run right after `leverage project init` and before `leverage project create`, since only project.yaml exists at that point (neither build.env nor config/common.tfvars do yet) and PathsHandler can't resolve a project name from either. leverage project init/create already skip PathsHandler unconditionally and need no change. For credentials configure, the top-level group callback now skips PathsHandler only in that specific corner case, and the credentials group callback (which already had a half-implemented branch for this exact scenario) writes build.env from project.yaml's short_name and builds real, project-scoped paths itself, so AWS credentials still land in ~/.aws/<project>/ instead of falling back to the global default location. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix | Stop AWS CLI probes from crashing credentials configure `leverage credentials configure --type BOOTSTRAP` crashed with "Command execution failed: The config profile (...) could not be found" on a first-ever run, because `_profile_is_configured` calls `Runner.exec(...)` without `raises=False`. `Runner.exec` defaults to raising an ExitError on any nonzero exit code, so the intended "probe, then report" logic (`return not exit_code`) never runs when the profile has never been configured (exit 255) - it aborts the whole command instead. This is a regression from the Docker-removal refactor (#316), which replaced the old non-raising `_exec` with the new `Runner`/`Runner.exec` (raises by default) without updating call sites written for the old contract. Six more `awscli.exec(...)` calls in credentials.py had the identical shape; `_credentials_are_valid` and `_get_organization_accounts` had real dead graceful-fallback logic, while `_get_management_account_id`, `_get_mfa_serial`, and the two `configure`/`configure_profile` key-setting loops only lost their more specific custom error message to Runner's generic one. All seven now explicitly pass `raises=False`. Hardened the `awscli_returning` test double to mimic Runner.exec's real raising contract, so a future regression of this shape fails its own test instead of passing silently, and added direct coverage for `_profile_is_configured` (previously untested). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix | Skip backing up an AWS config file that doesn't exist yet `leverage credentials configure --type BOOTSTRAP` crashed with a raw FileNotFoundError on ~/.aws/<project>/config the first time a profile's assumable-role setup ran, because `configure_accounts_profiles` unconditionally did `shutil.copy(paths.aws_config_file, ...)` with no existence check. `aws configure set` for access keys only ever writes the credentials file, never the config file - the config file only gets created once an account/role profile is configured for the first time, which is exactly what this function does. Its sibling backup in `configure_credentials` already guards this correctly with `if make_backup: ...`; this one didn't. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fixed common_tfvars and account_tfvars to return a Path instead of a str * Added Terraform Opentofu installer to test integration * Changed test layer * Changed test layer * Changed profile name * Fix | Cast MFA_ENABLED to a real boolean `PathsHandler.mfa_enabled` stored the raw string from build.env without casting it. Since any non-empty string (including "false") is truthy in Python, `elif paths.mfa_enabled:` in leverage/modules/auth.py always ran regardless of the actual value, so MFA_ENABLED=false never disabled the MFA credential-refresh path. This surfaced in CI as `refresh_layer_credentials_mfa` looking for a `[profile <name>-mfa]` section that was never meant to exist (the workflow explicitly sets MFA_ENABLED=false and sso_enabled=false to skip credential refresh entirely for that layer), and raising "Credentials for profile ... have not been properly configured." Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * Fix | Validate the actual layer, not cwd, in _validate_layout `_validate_layout` receives the specific layer Path to validate but called `paths.check_for_layer_location()` with no argument, which defaults to checking `paths.cwd` instead. Running `leverage terraform init --layers a,b` from an account-level "layers-group" directory (which itself has no .tf files, only its layer subdirectories do) made every layer's validation fail with "This command can only run at layer level.", even though the actual layers being validated were perfectly valid.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What?
Why?
References