Repository navigation
Fix native CI dogfood regressions - #2681
Merged
Merged
Conversation
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request. 
|
Dependency Review✅ No vulnerabilities or license issues found.Scanned FilesNone |
Contributor
|
Important Cloud Posse Engineering Team Review RequiredThis pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes. To expedite this process, reach out to us on Slack in the |
Erik Osterman (Cloud Posse) (osterman)
added
the
patch
Erik Osterman (Cloud Posse) (osterman)
marked this pull request as ready for review
July 2, 2026 19:37
Erik Osterman (Cloud Posse) (osterman)
requested a review
from a team
as a code owner
July 2, 2026 19:37
Resource Changes Found for
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR updates CI and workflow settings, adds CI bootstrap handling for no-arg git clone, extends Terraform backend path resolution, adds container network attachment and inspection support, updates emulator endpoint/network behavior, skips remote workdir lock persistence, and adds Aqua registry retry handling. ChangesDockerfile and Cache Action Validation
CI Git Clone Bootstrap Detection
Terraform Local Backend Path Override
Container Network Attachment and Emulator Network Reuse
Lock Persistence Skips Remote Workdir Sources
Aqua Registry Unauthenticated Retry
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Nitpick comments (7)
Dockerfile (1)
21-22: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueNit: double space before
--no-install-recommends.Line 22 has
apt-get -y install --no-install-recommendswith two spaces. Cosmetic only.🧹 Proposed fix
- apt-get -y install --no-install-recommends curl git ca-certificates python3; \ + apt-get -y install --no-install-recommends curl git ca-certificates python3; \🤖 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 `@Dockerfile` around lines 21 - 22, There is an extra space before the --no-install-recommends flag in the Dockerfile install command. Clean up the apt-get install line to use a single space there, keeping the command formatting consistent and matching the surrounding setup steps in the Dockerfile.pkg/provisioner/lock/lock_hook_test.go (1)
85-109: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the complementary positive-path test.
This test only covers the "skip" branch for remote sources. There's no test confirming persistence still succeeds through the
WorkdirPathKeypath for a local source (i.e.,SourceTypeLocalwith an existing directory reaching line 154'sreturn md.Source).As per coding guidelines, "whenever a test verifies that a recovery/fallback triggers under condition X, add a corresponding test that verifies the recovery does NOT trigger when condition X is absent."
🤖 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 `@pkg/provisioner/lock/lock_hook_test.go` around lines 85 - 109, Add the complementary positive-path test for autoLockProviders/lock_hook_test.go to cover the local-source case that should use WorkdirPathKey persistence. Mirror TestAutoLockProviders_SkipsRemoteWorkdirSource but set WorkdirMetadata.SourceType to SourceTypeLocal and use an existing local directory so the WorkdirPathKey path reaches the md.Source branch in the workdir resolver. Then assert that autoLockProviders records the expected lock-file write under the local source path, proving the skip logic only applies to remote sources.Source: Coding guidelines
pkg/provisioner/lock/lock_hook.go (1)
139-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSkip logic looks correct.
Remote sources short-circuit before the
os.Statcheck, and the stat/IsDir guard correctly limits persistence to existing local directories. No functional issues here.One gap: the
os.Stat/!info.IsDir()failure branch (lines 150-153) isn't exercised by the new test — only theSourceTypeRemoteskip path is covered. Consider adding a case wheremd.Sourcepoints to a missing path (or a file, not a dir) to lock in this branch's behavior.As per coding guidelines, "Every new feature must include comprehensive unit tests targeting >80% code coverage for all packages."
🤖 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 `@pkg/provisioner/lock/lock_hook.go` around lines 139 - 160, The new tests cover the remote-source skip path, but the missing-path/non-directory branch in persistDir is untested. Add a unit test around persistDir that sets workdir.ReadMetadata to return a local md.Source pointing to a missing path or file, then assert the function returns an empty string and logs the skip path; use the persistDir helper and the md.SourceType/ os.Stat guard scenario so this branch stays covered.Source: Coding guidelines
cmd/root_test.go (1)
172-183: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative-path test for the recovery branch.
TestHandleConfigInitError_AllowsCIGitCloneBootstraponly proves the recovery fires. It's worth adding a mirror case confirminghandleConfigInitErrordoes NOT swallow the error (and leavesCI.Enabledfalse) when the CI condition is absent — e.g.,GITHUB_ACTIONSunset or args are a normalgit clone repo. This guards against the bootstrap bypass silently activating outside CI.As per coding guidelines: "Include negative-path tests for recovery logic: whenever a test verifies that a recovery/fallback triggers under condition X, add a corresponding test that verifies the recovery does NOT trigger when condition X is absent."
🧪 Suggested negative-path test
func TestHandleConfigInitError_SkipsBootstrapOutsideCI(t *testing.T) { origArgs := os.Args t.Cleanup(func() { os.Args = origArgs }) os.Args = []string{"atmos", "--profile", "github", "git", "clone"} t.Setenv("GITHUB_ACTIONS", "") cfg := &schema.AtmosConfiguration{} err := handleConfigInitError(assert.AnError, cfg) assert.Error(t, err) assert.False(t, cfg.CI.Enabled, "bootstrap bypass must not activate outside CI") }🤖 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 `@cmd/root_test.go` around lines 172 - 183, Add a negative-path test alongside TestHandleConfigInitError_AllowsCIGitCloneBootstrap to verify handleConfigInitError does not recover when the CI bootstrap condition is absent. Create a case with a normal git clone-style argv and GITHUB_ACTIONS unset/empty, then assert the returned error is preserved and cfg.CI.Enabled remains false. Use the existing handleConfigInitError and schema.AtmosConfiguration symbols so the test mirrors the recovery path without triggering it.Source: Coding guidelines
pkg/toolchain/registry/aqua/aqua_test.go (1)
503-525: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid retry test — consider asserting the retry is truly unauthenticated.
The test confirms the retry fires (
calls == 2), but the server ignores auth, so it wouldn't catch a regression where the retried request still carriesAuthorization. Since dropping the token is the actual purpose of this fix, capturing the header per call makes the test guard what matters.♻️ Assert no Authorization header on the second request
ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { calls++ if calls == 1 { w.WriteHeader(http.StatusForbidden) return } + assert.Empty(t, r.Header.Get("Authorization"), "retry must be unauthenticated") w.Header().Set("Content-Type", "application/json") json.NewEncoder(w).Encode(releases) }))🤖 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 `@pkg/toolchain/registry/aqua/aqua_test.go` around lines 503 - 525, The retry test in TestAquaRegistry_GetLatestVersion_RetriesUnauthenticatedAfterForbidden only checks that a second request happens, but it does not verify that the retry drops authentication. Update the httptest.Server handler to inspect and record the Authorization header for each call, and assert that the first request is authenticated while the second request made by GetLatestVersion is unauthenticated after the 403 response. Use the existing ar.githubToken setup and the call-count logic to keep the test focused on the retry behavior.internal/terraform_backend/terraform_backend_local_test.go (1)
219-295: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid coverage — consider one more subtest to lock the contract.
Both subtests exercise only the happy path (relative path, file present). A quick add that pays off: a case where the configured
pathis set but the file is absent, assertingnil, nilis returned. If you also decide on the named-workspace behavior from the source-side comment, a subtest pinning that down here would keep the contract explicit.🤖 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 `@internal/terraform_backend/terraform_backend_local_test.go` around lines 219 - 295, Add a subtest in TestReadTerraformBackendLocal_ConfiguredBackendPath that sets a configured backend path but does not create the state file, and assert ReadTerraformBackendLocal returns nil content with a nil error. Use the existing ReadTerraformBackendLocal and ProcessTerraformStateFile helpers as the anchors for locating the test, and if you are also fixing workspace handling, add a separate subtest to pin the named-workspace behavior so the contract stays explicit.pkg/emulator/endpoint_host.go (1)
78-90: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winConsider excluding the default
bridgenetwork too.
firstReachableNetworkskips"","host","none"but not"bridge", Docker's default network. If a container ends up attached to bothbridgeand an intentional custom job network,bridgeis a valid pick here even though it typically doesn't support the kind of alias-based container-to-container resolution this feature relies on. Combined with the nondeterministic ordering fromgetNetworksFromInspect/parsePodmanNetworks(see comments on those files), this could route emulator lookups to the wrong network.🤖 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 `@pkg/emulator/endpoint_host.go` around lines 78 - 90, firstReachableNetwork currently treats bridge as a usable network, which can cause emulator lookups to pick Docker’s default network instead of the intended job network. Update the firstReachableNetwork function to also skip "bridge" alongside "", "host", and "none", so it only returns a network that is suitable for alias-based container resolution.
🤖 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 `@pkg/container/docker.go`:
- Around line 261-278: getNetworksFromInspect returns network names by ranging
over a map, so the order is nondeterministic and can make firstReachableNetwork
in endpoint_host.go pick different networks between runs. Update
getNetworksFromInspect to collect the names from NetworkSettings.Networks, then
sort the resulting slice before returning it so Info.Networks is stable and
deterministic.
In `@pkg/container/podman.go`:
- Around line 277-284: The map[string]interface{} branch in parsePodmanNetworks
builds the network list by ranging over a map, so its order is nondeterministic
and can change which network firstReachableNetwork selects. Update
parsePodmanNetworks to produce a stable, sorted slice before returning it,
matching the deterministic behavior needed by firstReachableNetwork and the
existing docker.go handling.
In `@pkg/emulator/endpoint_host_test.go`:
- Around line 3-10: The import block in endpoint_host_test.go needs to be split
into the standard three groups with blank lines between them: stdlib imports
first, then third-party imports, then Atmos packages. Reorder the imports in the
test file so github.com/stretchr/testify/assert stays in the third-party group
and github.com/cloudposse/atmos/pkg/container moves to the Atmos group, with
context, os, and testing kept together in the stdlib group.
In `@pkg/emulator/endpoint_host.go`:
- Around line 3-13: The import block in endpoint_host.go is grouped incorrectly:
Atmos packages like container and perf are mixed with the 3rd-party viper
import. Reorder the imports in the package’s import section so they are split
into three blank-line-separated groups: stdlib imports first, then 3rd-party
imports such as viper, and finally Atmos imports like container and perf,
keeping each group alphabetized.
In `@pkg/emulator/manager.go`:
- Around line 257-260: The current-container-network path can collide because
emulatorNetworkAlias(name) only uses the component name, so different stacks can
register the same network alias on the shared network. Update the alias
generation to include the stack identifier for this path, or change the
current-container-network flow in manager.go to use per-stack networks so
aliases remain unique; use emulatorNetworkAlias and the network setup logic
together when applying the fix.
In `@pkg/provisioner/lock/lock_hook_test.go`:
- Around line 107-108: The current `lock_hook_test.go` assertion uses a
repo-relative `filepath.Join(...)`, so a regression in `persistDir`/`copyLock`
could create real `github.com/...` directories in the checkout instead of
staying inside the temp area. Update the test to anchor path resolution to
`t.TempDir()` or `t.Chdir(dir)`, or assert directly on `persistDir(cc,
workdirPath)` returning empty for remote sources, so the check remains
side-effect free while still validating `os.Stat`/`os.IsNotExist` behavior.
---
Nitpick comments:
In `@cmd/root_test.go`:
- Around line 172-183: Add a negative-path test alongside
TestHandleConfigInitError_AllowsCIGitCloneBootstrap to verify
handleConfigInitError does not recover when the CI bootstrap condition is
absent. Create a case with a normal git clone-style argv and GITHUB_ACTIONS
unset/empty, then assert the returned error is preserved and cfg.CI.Enabled
remains false. Use the existing handleConfigInitError and
schema.AtmosConfiguration symbols so the test mirrors the recovery path without
triggering it.
In `@Dockerfile`:
- Around line 21-22: There is an extra space before the --no-install-recommends
flag in the Dockerfile install command. Clean up the apt-get install line to use
a single space there, keeping the command formatting consistent and matching the
surrounding setup steps in the Dockerfile.
In `@internal/terraform_backend/terraform_backend_local_test.go`:
- Around line 219-295: Add a subtest in
TestReadTerraformBackendLocal_ConfiguredBackendPath that sets a configured
backend path but does not create the state file, and assert
ReadTerraformBackendLocal returns nil content with a nil error. Use the existing
ReadTerraformBackendLocal and ProcessTerraformStateFile helpers as the anchors
for locating the test, and if you are also fixing workspace handling, add a
separate subtest to pin the named-workspace behavior so the contract stays
explicit.
In `@pkg/emulator/endpoint_host.go`:
- Around line 78-90: firstReachableNetwork currently treats bridge as a usable
network, which can cause emulator lookups to pick Docker’s default network
instead of the intended job network. Update the firstReachableNetwork function
to also skip "bridge" alongside "", "host", and "none", so it only returns a
network that is suitable for alias-based container resolution.
In `@pkg/provisioner/lock/lock_hook_test.go`:
- Around line 85-109: Add the complementary positive-path test for
autoLockProviders/lock_hook_test.go to cover the local-source case that should
use WorkdirPathKey persistence. Mirror
TestAutoLockProviders_SkipsRemoteWorkdirSource but set
WorkdirMetadata.SourceType to SourceTypeLocal and use an existing local
directory so the WorkdirPathKey path reaches the md.Source branch in the workdir
resolver. Then assert that autoLockProviders records the expected lock-file
write under the local source path, proving the skip logic only applies to remote
sources.
In `@pkg/provisioner/lock/lock_hook.go`:
- Around line 139-160: The new tests cover the remote-source skip path, but the
missing-path/non-directory branch in persistDir is untested. Add a unit test
around persistDir that sets workdir.ReadMetadata to return a local md.Source
pointing to a missing path or file, then assert the function returns an empty
string and logs the skip path; use the persistDir helper and the md.SourceType/
os.Stat guard scenario so this branch stays covered.
In `@pkg/toolchain/registry/aqua/aqua_test.go`:
- Around line 503-525: The retry test in
TestAquaRegistry_GetLatestVersion_RetriesUnauthenticatedAfterForbidden only
checks that a second request happens, but it does not verify that the retry
drops authentication. Update the httptest.Server handler to inspect and record
the Authorization header for each call, and assert that the first request is
authenticated while the second request made by GetLatestVersion is
unauthenticated after the 403 response. Use the existing ar.githubToken setup
and the call-count logic to keep the test focused on the retry behavior.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bcbdf4fb-5306-4317-94b1-62dd029d03c8
📒 Files selected for processing (24)
Dockerfileactions/cache/action.ymlcmd/docker_and_action_regression_test.gocmd/root.gocmd/root_test.gointernal/terraform_backend/terraform_backend_local.gointernal/terraform_backend/terraform_backend_local_test.gopkg/container/common.gopkg/container/common_test.gopkg/container/docker.gopkg/container/docker_unit_test.gopkg/container/lifecycle.gopkg/container/podman.gopkg/container/podman_test.gopkg/container/runtime.gopkg/emulator/endpoint_host.gopkg/emulator/endpoint_host_test.gopkg/emulator/manager.gopkg/emulator/manager_more_test.gopkg/emulator/manager_test.gopkg/provisioner/lock/lock_hook.gopkg/provisioner/lock/lock_hook_test.gopkg/toolchain/registry/aqua/aqua.gopkg/toolchain/registry/aqua/aqua_test.go
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #2681 +/- ##
==========================================
+ Coverage 80.58% 80.60% +0.02%
==========================================
Files 1524 1525 +1
Lines 144122 144360 +238
==========================================
+ Hits 116142 116365 +223
Misses 21466 21466
- Partials 6514 6529 +15
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
|
Note SHA Pin Verification Passed ✅All 101 SHA-pinned action(s) verified against upstream tags. |
Andriy Knysh (aknysh)
approved these changes
Jul 3, 2026
Contributor
|
Tip Atmos Pro
No affected stacks workflow was detected for this pull request.
|
|
These changes were released in v1.223.0-rc.1. |
This was referenced Aug 15, 2026
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
atmos git clonecan run before repo-local profile/config files exist, and make the cache action fail fast when Atmos cache metadata is missing.pathstate reads, remote source-provisioned lock persistence, Dockerpython3, Aqua latest lookup fallback, and emulator job-container networking.docker run --network hostwrapper.why
references