fix(deploy): don't wrap down/ps/logs compose calls in op run - #73
Merged
Conversation
The "Teardown removed stacks" step runs before the git reset, so it reads the previous compose.env. When a commit removes a stack and also drops a now-orphaned var whose 1Password item was already archived, `op run` fails to resolve that dead ref and aborts — before `docker compose down` executes. The removed stack's containers are never stopped (the failure is only a ::warning::, so the job stays green), and CI can't self-heal on a later run because the stack is no longer detected as removed. `docker compose down`/`ps`/`logs` operate on already-created resources by compose project label and need no resolved secrets — only `up` genuinely requires op run to inject env into new containers. Every non-`up` call had inherited the op run wrapper by copy-paste consistency (git history shows no secrets-driven rationale in any of the introducing commits; the adjacent `docker inspect` calls were already unwrapped). Strip `op run --env-file` from all 17 inline down/ps/logs calls, leaving the 4 `up` calls wrapped. Decouples teardown and failure diagnostics from compose.env resolvability so a dead ref can't strand containers or hide diagnostics.
Reviewer's GuideRemoves 1Password Sequence diagram for deploy teardown and diagnostics without op run wrappingsequenceDiagram
actor Developer
participant GitHubActions as GitHub_Actions_deploy_workflow
participant ComposeEnv as compose_env
participant OnePassword as op
participant DockerCompose as docker_compose
Developer->>GitHubActions: push commit removing stack and vars
GitHubActions->>ComposeEnv: read_previous_compose_env
alt removed_stack_teardown
GitHubActions->>DockerCompose: docker compose down
DockerCompose-->>GitHubActions: containers_stopped_by_project_label
end
alt diagnostics_ps_logs
GitHubActions->>DockerCompose: docker compose ps -a
DockerCompose-->>GitHubActions: container_status_json
GitHubActions->>DockerCompose: docker compose logs --tail
DockerCompose-->>GitHubActions: container_logs
end
opt deploy_new_stack_up
GitHubActions->>OnePassword: op run --env-file compose.env
OnePassword->>DockerCompose: docker compose up
DockerCompose-->>GitHubActions: new_containers_started_with_secrets
end
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
There was a problem hiding this comment.
Hey - I've left some high level feedback:
- The long explanatory comment about why
down/ps/logsare not wrapped inop runis helpful but quite dense; consider tightening it or extracting the rationale into a short, referenced note to keep the workflow readable. - The diagnostics helper (
docker compose ps+logs+docker inspect) is duplicated in multiple steps; consider factoring it into a reusable shell function or separate action to reduce repetition and keep future changes consistent. - Now that
op runis removed fromdocker compose pscalls, ensure that any reliance on env-derivedCOMPOSE_PROJECT_NAMEor other compose env settings is explicitly set elsewhere, or document that the default project naming is sufficient for these commands.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The long explanatory comment about why `down`/`ps`/`logs` are not wrapped in `op run` is helpful but quite dense; consider tightening it or extracting the rationale into a short, referenced note to keep the workflow readable.
- The diagnostics helper (`docker compose ps` + `logs` + `docker inspect`) is duplicated in multiple steps; consider factoring it into a reusable shell function or separate action to reduce repetition and keep future changes consistent.
- Now that `op run` is removed from `docker compose ps` calls, ensure that any reliance on env-derived `COMPOSE_PROJECT_NAME` or other compose env settings is explicitly set elsewhere, or document that the default project naming is sufficient for these commands.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Address Sourcery review: condense the rationale comment and document that default compose project naming (stack dir basename) is what down/ps/logs rely on — callers set no COMPOSE_* vars, so dropping op run doesn't change which project these commands target.
Owner
Author
|
Thanks @sourcery-ai — addressed:
|
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
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.
Problem
The Teardown removed stacks step runs before the
git reset --hard(it needs the pre-reset compose files to tear down stacks the commit deleted). So it reads the previouscompose.env.When a commit both removes a stack and drops a now-orphaned var whose 1Password item was already archived,
op runresolves the entire env-file up front, hits the dead ref, and aborts beforedocker compose downruns. Result:::warning::down failed for <stack>, so the deploy job stays green.prepareno longer detects the stack as removed (previous_sha == target), so the teardown step won't fire again. Cleanup then requires a manualdocker compose -p <stack> downon the host.This actually happened on
docker-zendc: media/logging/librarymanager were left running ~38h after their "successful" removal deploy, because the pre-resetcompose.envstill referenced the long-archivedzendc-zencommanditem.Fix
docker compose down/ps/logsoperate on already-created resources by compose project label and need no resolved secrets. Onlyupgenuinely requiresop run(it injects env into new containers).This strips
op run --env-filefrom all 17 inlinedown/ps/logscalls, leaving the 4upcalls wrapped. A${VAR}in a torn-down file simply resolves to empty (no${VAR:?}guards exist in any caller compose file; all networks areexternal: true, whichdownnever removes), so teardown and failure-diagnostics no longer depend oncompose.envbeing fully resolvable.Why this is safe / not a regression
Git history shows no secrets-driven rationale in any commit that introduced these wrapped calls:
ps/logs(8a812a3) — a diagnostics feature;op runapplied uniformly alongside theupin the same helper.down(301c863,406ed43, self-hosted rewrite) — messages are about clearing orphans, never secrets.docker inspectcalls were already unwrapped — the author already knew label/name-based inspection needs noop run.Verification
yamllint --strict✅actionlint(includes shellcheck onrun:blocks) ✅timeout … op run … -- \+docker compose upcalls retainop run; 0 inline wrapped calls remain.Non-breaking (no input changes) — callers pick it up via Renovate's SHA-pin bump.
Summary by Sourcery
Simplify deploy workflow Docker Compose interactions to avoid teardown failures caused by unresolved 1Password env references.
Enhancements:
op runwrapping from all Docker Composedown/ps/logscalls in the deploy workflow while keepingupcalls wrapped for secret injection.op runto ensure reliable stack cleanup.CI:
compose.envvia 1Password.