[AI-6917] Smarter ddev release branch tag command#23860
Open
AAraKKe wants to merge 11 commits into
Open
Conversation
…`--yes` The command previously required the user to be on the release branch and only let them pick between `--rc` and `--final`. The new flags make it possible to tag a release branch from anywhere, pin or auto-suggest the RC number, and tag an arbitrary commit instead of the branch tip: - `--release/-r <X.Y[.x]>` selects the target release branch; if not on it, a worktree is created at `.worktrees/release-tag/<branch>` from `origin/<branch>` and torn down on success. - `--ref <commit>` tags the given commit (must be an ancestor of `origin/<branch>`) instead of the branch tip. - `--rc` accepts an optional value: `--rc` alone auto-suggests, `--rc N` pins. Warns when N skips ahead of the next available RC. - `--yes/-y` skips all yes/no confirms.
|
- `GitRepository.tag()` now uses its `message` parameter instead of the tag name (pre-existing bug: `--message` was always set to `value`). - Compute `current_branch` once and thread it into `_resolve_target_branch` to avoid the duplicate `git rev-parse` subprocess call. - Move worktree setup inside the try/finally so any failure between worktree creation and teardown still triggers the inspection warning. - Narrow the OSError catch to just `git.tag` / `git.push` so other failures surface with their own attributable messages. - `_branch_exists_on_origin` no longer swallows OSError as "branch missing" — split into `_ensure_branch_on_origin` that distinguishes a real ls-remote failure from an empty result. - Confirmation prompt for `--ref` shows the resolved commit SHA, not the raw user input, so the user can verify what `rev-parse` resolved to. - Renamed `_create_or_refresh_worktree` to `_create_worktree` (it only adds) and the failure message now hints at the manual cleanup command. - Added an `exit_code` assertion to `test_no_worktree_when_already_on_target_branch`. - Input-validation helpers now raise `click.UsageError` so mypy follows the `NoReturn` semantics; updated the two affected tests to assert non-zero exit instead of exit code 1 (UsageError exits with 2).
- Drop unused `app` arg from `_parse_rc_value` (it raises `click.UsageError` directly and never needed application state). - Include the underlying git error text in the `--ref` failure messages for both `rev-parse` and `merge-base --is-ancestor`. - Remove dead post-`range` guard in `_warn_on_rc_gap`. - Annotate `_check_open_prs` return as `list[PullRequest]`. - Rename changelog entry from `.added` to `.changed`: the new confirm prompt on a release branch is a behavior change for non-interactive callers that piped a single `y`. - Replace the `capture.return_value` fixture default with a side_effect callable that dispatches on the first argument, so new code paths that call `capture()` without an explicit override don't silently inherit the `ls-remote` payload. - Use a `_make_ref_dispatcher` helper in `--ref` tests so the mocked responses are keyed by subcommand instead of call order. - Extract `_worktree_subcommands(git, sub)` helper to dedupe the worktree-call assertions.
Contributor
|
|
- `--final` + `--rc` now raises `click.UsageError` (exit code 2) to match the other CLI-validation errors in the command. - Change the `RC_AUTO` sentinel to a private, unguessable string so `--rc auto` is no longer silently equivalent to bare `--rc`. - Merge `_resolve_tag_ref`'s two try/except blocks; the branch that picks the abort message keys off whether `commit_sha` was bound, which avoids the possibly-unbound warning some type-checkers emit. - `_warn_on_rc_gap` now uses `app.display_warning` to match the other new helpers in this PR. - Test additions: full call-shape assertions on the `worktree add` / `worktree remove` invocations, plus a new test that covers the `_create_worktree` abort path and its manual-cleanup hint.
The command body had four logical phases interleaved with the worktree lifecycle. Each phase is now its own helper: - `_prepare_working_dir` decides between pull-in-place and worktree creation, returns `(git, working_dir, worktree_created)`. - `_warn_if_build_agent_yaml_stale` surfaces the recovery warning and returns whether the workflow dispatch is needed. - `_compute_new_tag` owns the existing-tag collection, RC resolution, and all RC validations (bounds, exists, gap, backward move). - `_confirm_and_push_tag` owns the open-PR check, the final confirm prompt, and the `git tag` / `git push` pair. The main `tag()` body shrinks from ~108 to ~35 lines and reads top-to-bottom as the steps of a tagging operation. The worktree `try/finally` stays in the main body — a future change can replace it with a context manager once worktree management is consolidated across the codebase.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0c1f88891e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| app.display_waiting(f'Creating worktree at `{worktree_path}` from `origin/{branch}`...') | ||
| worktree_path.parent.ensure_dir_exists() | ||
| try: | ||
| app.repo.git.run('worktree', 'add', '-B', branch, str(worktree_path), f'origin/{branch}') |
There was a problem hiding this comment.
When --release is run from any branch other than the release branch and the user already has a local 7.56.x branch that is not checked out, this git worktree add -B ... origin/7.56.x resets that local branch to the remote tip before tagging. I checked git worktree add -h, where -B <branch> is described as “create or reset a branch”, so local-only commits on that branch lose their branch ref even though the command is only preparing a temporary worktree; use a detached worktree or a throwaway branch name instead.
Useful? React with 👍 / 👎.
The previous design checked out the target branch in a worktree under `.worktrees/release-tag/<branch>/` and ran the tag command from there. The worktree was only ever needed to read `.gitlab/build_agent.yaml` from the right commit, since every other step (listing tags, validating `--ref`, creating and pushing the tag) keys off ref/object names that any clone resolves the same way. Read the YAML via `git show <ref>:.gitlab/build_agent.yaml` and pass an explicit `ref` to `git tag` (`origin/<target>` by default, the resolved commit when `--ref` is supplied). The user's local checkout and branches are never touched. Side effects: - Drops `WORKTREE_BASE`, `_create_worktree`, `_remove_worktree`, `_prepare_working_dir`, and the worktree `try/finally`. - Closes a silent-data-loss bug: `git worktree add -B <branch>` force-resets a pre-existing local `<branch>` to the remote tip, orphaning any local-only commits on it. - Reads the YAML at the actual commit being tagged when `--ref` is supplied, rather than at the branch tip. - Removes the implicit `git pull origin <branch>` that the no-worktree path used to do — tagging no longer mutates local branch state under any flag combination.
Collaborator
Author
|
@codex review |
|
Codex Review: Didn't find any major issues. 🎉 ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
- Move the `< 1` guard into `_parse_rc_value` so `--rc 0` is rejected via `click.UsageError` (exit code 2) like every other `--rc` parse failure, instead of via `app.abort` (exit code 1) after the prompt branch. The interactive-prompt branch still aborts on `< 1`, which is now the only path that reaches that check. - Add `git` as an explicit parameter to `_ensure_branch_on_origin` to match the other git-using helpers. - Tighten the worktree-regression guard in tests: filter both `run` and `capture` calls, since `git worktree` operations can go through either entry point. - `_make_ref_dispatcher` now delegates to `_capture_dispatch` for subcommands it doesn't explicitly handle, removing the duplicated `ls-remote` literal.
Contributor
Validation ReportAll 21 validations passed. Show details
|
AAraKKe
changed the title
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
1 participant
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 does this PR do?
Adds
--release/-r,--ref,--rc N, and--yes/-ytoddev release branch tag. The command can now target any release branch from anywhere, tag an arbitrary commit instead of the branch tip, pin the RC number explicitly, and skip yes/no prompts. Tagging operates againstorigin/<branch>directly — the user's local branches and worktree are never touched.Motivation
The previous command required the user to be on the release branch and only chose between
--rcand--final. Tagging a specific commit (e.g. when later backports should not be included), filling in a missed RC number, or releasing from a different branch all required manual git work outside the tool.Review checklist (to be filled by reviewers)
This PR has been created and validated using the paired-review skill from agent-integrations. Ready for human review.