Commit db5fc86
committed
![pull[bot]](https://avatars.githubusercontent.com/in/12910?v=4&size=40){kind=avatar}
Fetch commit from origin when not in local repo for ddev port-commit (DataDog#23703)
* Resolve ddev port-commit input by fetching from origin when not local
When the commit passed to `ddev release port-commit` is not in the local
object database, fall back to `git fetch origin <sha>` before aborting.
GitHub allows fetching any reachable commit by SHA, so this lets us port
commits that live on branches the local `remote.origin.fetch` refspec
does not track (a common configuration in this repo).
The fetch is precise — we never need to know which branch the commit is
on, since cherry-pick only needs the commit object.
* Add changelog entry for DataDog#23703
* Abort early on abbreviated SHAs and clarify test names
Address review feedback:
- Abbreviated SHAs cannot be fetched from GitHub via uploadpack
(allowReachableSHA1InWant only honours full 40-char SHAs), so detect
the abbreviated-hex case before attempting the fetch and abort with a
clear "pass the full 40-character SHA" message. Avoids the confusing
`fatal: couldn't find remote ref` stderr from git on a doomed fetch.
- Rename `test_command_aborts_when_commit_does_not_exist` to
`test_command_aborts_when_commit_missing_after_fetch`. The original
name suggested it covered the "commit truly missing" case, but with
the mock setup it actually exercises the edge where the fetch
succeeds but rev-parse still fails. The new name reflects that.
- Add `test_command_aborts_on_abbreviated_sha_not_local` for the new
early-abort path.
- Use a full 40-char SHA constant for the fetch-fallback tests so they
reach the fetch path under the new early-abort check.
* Use contextlib.suppress instead of except: pass
* Annotate Application.abort as NoReturn
The original `app.abort` calls `ctx.exit` (which raises `SystemExit`),
so the method never returns. Mark it with `-> NoReturn` so callers
don't need a trailing `raise AssertionError('unreachable')` to satisfy
mypy when the abort is the last statement on a code path. Also add an
explicit `raise SystemExit(code)` so mypy can see the no-return at the
implementation level (the `__exit_func` field is untyped) and as a
safety net if a buggy `exit_func` is ever passed in.
Removes the two `raise AssertionError('unreachable')` lines from
`_resolve_commit_or_fetch` that were only there for type checking.
* Type Application.exit_func and abort so callers don't need a trailing raise
Annotate the `exit_func` callback as `Callable[[int], NoReturn]`
(matching click's `Context.exit`) and mark `Application.abort` as
`-> NoReturn`. Mypy now infers that calling `self.__exit_func(code)`
doesn't return, so the abort path is correctly recognised as terminal
without needing an unreachable `raise SystemExit(code)` inside the
method body.
Typing the `__init__` body surfaced two pre-existing issues:
- `self.__config = {}` needed an explicit `dict[str, Any]` annotation.
- `serve_openmetrics_payload.py` was constructing `DockerAgent` with
`app.platform` (a `Platform`) where it expects an `Application`.
This has been broken since 2024 — `AgentInterface.platform` accesses
`self.app.platform`, which would raise `AttributeError` on a Platform
instance at runtime. Pass `app` directly.
Typing-only imports (`Any`, `Callable`, `NoReturn`) live behind
`TYPE_CHECKING` since the file already uses `from __future__ import
annotations`.
* Skip fetch fallback in --dry-run mode
Preserves the dry-run contract: when the commit is not in the local
object database, abort with a clear message instead of running
`git fetch origin <sha>` during plan resolution. Tells the user to
re-run without --dry-run or pre-fetch the commit manually.
Adds `test_command_aborts_in_dry_run_when_commit_not_local` covering
the new path and asserting no fetch is attempted.
* Add PR-or-commit input support to ddev port-commit
The command now accepts a PR number, an explicit `PR-<number>` token, or
a GitHub PR URL in addition to a commit SHA. Detection is tiered:
- `PR-<digits>` or a PR URL is treated as an explicit PR reference.
- Pure-digit inputs are tried as a PR first when a GitHub token is
configured; a 404 falls back to commit resolution.
- Anything else goes straight to commit resolution.
When the input resolves to a PR, the command verifies the PR is merged
and that its merge commit has a single parent (i.e. the PR was
squash-merged); anything else aborts with a clear message instructing
the user to pass the specific commit they want to backport.
Refactors `_resolve_commit_or_fetch` to raise `_CommitNotResolvable`
instead of aborting, so the new orchestrator can wrap the error with a
unified "PR or commit" message when pure-digit input also fails commit
resolution. The single abort happens at the call site in
`resolve_port_plan`.
Adds:
- `AsyncGitHubClient.get_pull_request` for the GitHub lookup.
- `FakeAsyncGitHubClient.get_pull_request` plus a built-in 404 default
so tests that don't care about PR lookup auto-fall-through.
- New regex constants and helpers.
- `[COMMIT_OR_PR]` metavar and updated docstring on the click command.
* Add tests and changelog entry for PR-or-commit input
* Address round-1 review feedback for PR-or-commit input
Apply reviewer findings on top of the PR-or-commit input change:
- Wrap `_resolve_commit_or_fetch` inside `_resolve_pr_to_commit` so a
non-resolvable merge commit reports `PR #N ...` instead of leaking the
raw SHA the user never typed.
- Handle `pydantic.ValidationError` from `get_pull_request`, and split
401 / 403 out of the generic HTTP error arm with an actionable hint
about `github.token`.
- Move the abbreviated-SHA check ahead of the dry-run gate in
`_resolve_commit_or_fetch` so abbreviated input gives the real
diagnosis on the first run instead of forcing a retry.
- Guard the `rev-list --parents` call in `_abort_if_merge_commit`
against `OSError`, matching the rest of the module's careful style.
- Tighten the token-required abort for explicit PR inputs to flag the
`--no-pr` carve-out (the lookup is needed regardless of PR creation).
- Drop `_fetch_pr`'s dependency on the full `Application`; only the
token is needed.
- Annotate `Application.abort` parameters (`text: str`, `code: int`,
`**kwargs: Any`) to finish the typing surface.
- Strip the raw input in `_resolve_input` so trailing whitespace from a
copy-paste no longer drops the user to commit resolution.
- Drop `[ ]` from the click metavar; click adds them for optional
arguments and they were rendering doubled in --help.
- Replace the test helper's `lambda` exit function with a real
`NoReturn` callable so it matches the new `Application.__init__`
signature. Also corrects the pre-existing `Applicatione` typo.
- Add `http://`, fragment, and trailing-whitespace variants to the
parametrized PR-input test.
- Remove an em-dash from a test comment.
* Address round-2 review feedback
- Convert `_abort_if_merge_commit`'s try/except to try/else so the
parent-count binding is structurally explicit rather than relying on
`Application.__exit_func` being `NoReturn` at runtime. The typing
contract still holds, but a future caller wiring a non-terminating
exit function would now surface as a structural error instead of an
`UnboundLocalError`.
- Add `test_command_emits_pr_context_when_pure_digit_pr_merge_commit_missing`
pinning the path the round-1 f-1 fix was filed for: pure-digit input
+ PR found + non-resolvable merge commit should abort with a message
that names `PR #N`, not the raw SHA the user never typed.
* Drop redundant negative assertion in PR-context regression test
The positive assertion already validates that the PR-context message
fires. The negative assertion was meant to guard against the raw
commit-resolution message firing alone, but that message is in fact
suffixed onto the PR-context wrap, so any `not in` / `startswith` form
either passed trivially (Rich console wrapping) or failed for the
wrong reason. Drop it.
* Consolidate the PR's changelog into a single entry
* Quiet httpx and show a progress line during PR resolution
The httpx logger emits an INFO line for every request, which leaks
through to the CLI as `INFO: HTTP Request: GET ... "HTTP/1.1 200 OK"`
just before our own status messages. Drop httpx to WARNING at command
entry so only failures are surfaced.
Also print `Resolving PR #N via GitHub...` before the lookup so the
user sees something happen when the command pauses for the API call,
matching the existing "Commit X not found locally; fetching from
origin" pattern.
* Force `git add` when resolving `.in-toto/` conflicts
`.in-toto/` is gitignored in this repo (and most repos that ship signed
packages), so `git add <path>` refuses to stage the file even though
the path is already tracked in HEAD. `_resolve_in_toto_conflict` gets
here only for paths git itself flagged as `--diff-filter=U`, so forcing
the add is safe and matches the rest of the function's "keep target
branch's .in-toto" intent.
Without this, cherry-picking any commit that touches a `.in-toto/`
file (i.e. every release commit) aborts mid-workflow with `Command
'['git', 'add', '.in-toto/...']' returned non-zero exit status 1`. d54c6b81 parent 0fe93e0 commit db5fc86
2 files changed
Lines changed: 2 additions & 2 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
137 | 137 | | |
138 | 138 | | |
139 | 139 | | |
140 | | - | |
| 140 | + | |
141 | 141 | | |
142 | 142 | | |
143 | 143 | | |
| |||
Large diffs are not rendered by default.
0 commit comments