potiuk commented on code in PR #66255:
URL: https://github.com/apache/airflow/pull/66255#discussion_r3177102651
##########
dev/breeze/src/airflow_breeze/utils/github.py:
##########
@@ -35,6 +37,81 @@
if TYPE_CHECKING:
from requests import Response
+GITHUB_TOKEN_ENV_VARS = ("GH_TOKEN", "GITHUB_TOKEN")
+
+
+def env_without_github_tokens(env: Mapping[str, str] | None = None) ->
dict[str, str]:
+ """Return a copy of *env* with ambient GitHub CLI token variables
removed."""
+ cleaned_env = dict(os.environ if env is None else env)
+ for token_env_var in GITHUB_TOKEN_ENV_VARS:
+ cleaned_env.pop(token_env_var, None)
+ return cleaned_env
+
+
+def get_github_token_from_env(env: Mapping[str, str] | None = None) -> str |
None:
+ """Return an ambient GitHub token using the same precedence as the GitHub
CLI."""
+ source_env = os.environ if env is None else env
+ for token_env_var in GITHUB_TOKEN_ENV_VARS:
+ token = source_env.get(token_env_var)
+ if token:
+ return token
+ return None
+
+
+def run_gh_command(
+ command: Sequence[str],
+ *,
+ retry_with_github_token: bool = True,
+ env: Mapping[str, str] | None = None,
+ **kwargs: Any,
+) -> subprocess.CompletedProcess[Any]:
+ """
+ Run a ``gh`` command using stored ``gh auth login`` credentials before
ambient token env vars.
+
+ Locally, ``GH_TOKEN``/``GITHUB_TOKEN`` can shadow the user's normal GitHub
CLI login. We first
+ run with those variables removed, then retry with the original environment
only when that fails.
+ """
+ command_env = os.environ.copy() if env is None else dict(env)
+ check = kwargs.pop("check", False)
+ result = subprocess.run(command,
env=env_without_github_tokens(command_env), check=False, **kwargs)
+ if result.returncode == 0:
+ return result
+ if not retry_with_github_token or not
get_github_token_from_env(command_env):
+ if check:
+ result.check_returncode()
+ return result
+ return subprocess.run(command, env=command_env, check=check, **kwargs)
+
+
+def retrieve_github_token(token: str | None = None, *, env: Mapping[str, str]
| None = None) -> str | None:
+ """
+ Resolve a GitHub token for local Breeze commands.
+
+ Explicit ``--github-token`` values are preserved. Ambient
``GH_TOKEN``/``GITHUB_TOKEN`` values are
+ used only after trying the user's stored ``gh auth login`` credential.
+ """
+ env_token = get_github_token_from_env(env)
+ source_env = os.environ if env is None else env
+ env_tokens = {
+ source_env[token_env_var] for token_env_var in GITHUB_TOKEN_ENV_VARS
if source_env.get(token_env_var)
+ }
+ if token and token not in env_tokens:
Review Comment:
This rule — "if the explicit `token` matches an env-var value, treat it as
not-explicit and re-resolve from `gh auth login`" — is the right defense
against Click auto-populating from `envvar=("GITHUB_TOKEN", "GH_TOKEN")`, but
it will surprise anyone who genuinely passes `--github-token "$GITHUB_TOKEN"`
from a script.
Two suggestions:
- Spell the rule out in the docstring (today it just says "explicit values
are preserved", which isn't quite true).
- Worth checking the `--github-token` Click options across the five callers
— if none of them set `envvar=...`, this defense isn't needed and we can trust
the explicit arg directly.
---
Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before posting
##########
dev/breeze/src/airflow_breeze/commands/workflow_commands.py:
##########
@@ -127,24 +127,19 @@ def workflow_run_publish(
)
sys.exit(1)
if os.environ.get("GITHUB_TOKEN", ""):
- console_print("\n[warning]Your authentication will use GITHUB_TOKEN
environment variable.")
+ console_print("\n[warning]GITHUB_TOKEN environment variable is set.")
Review Comment:
Phrasing nit: "`GITHUB_TOKEN` environment variable is set" buries the lede.
Consider something more action-oriented, e.g.:
> `GITHUB_TOKEN` is set; Breeze will try your `gh auth login` first and only
use this token as a fallback. The fallback token must have workflow-trigger
scope.
The previous "If you remove GITHUB_TOKEN..." advice is no longer needed
(which is the whole point of this PR), but the new message could be clearer
about *what changed*.
---
Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before posting
##########
dev/breeze/tests/test_github_utils.py:
##########
@@ -0,0 +1,122 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+from __future__ import annotations
+
+import subprocess
+from unittest import mock
+
+from airflow_breeze.utils.github import (
+ env_without_github_tokens,
+ retrieve_github_token,
+ run_gh_command,
+)
+
+
+def _completed_process(returncode: int, stdout: str = "") ->
subprocess.CompletedProcess[str]:
+ return subprocess.CompletedProcess(args=["gh"], returncode=returncode,
stdout=stdout, stderr="")
+
+
+def test_env_without_github_tokens_removes_ambient_token_vars(monkeypatch):
+ monkeypatch.setenv("GH_TOKEN", "gh-token")
+ monkeypatch.setenv("GITHUB_TOKEN", "github-token")
+ monkeypatch.setenv("OTHER_VAR", "kept")
+
+ cleaned_env = env_without_github_tokens()
+
+ assert "GH_TOKEN" not in cleaned_env
+ assert "GITHUB_TOKEN" not in cleaned_env
+ assert cleaned_env["OTHER_VAR"] == "kept"
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def test_retrieve_github_token_prefers_clean_gh_auth_token(mock_run,
monkeypatch):
+ monkeypatch.setenv("GH_TOKEN", "env-gh-token")
+ monkeypatch.setenv("GITHUB_TOKEN", "env-github-token")
+ mock_run.return_value = _completed_process(returncode=0,
stdout="stored-gh-token\n")
+
+ assert retrieve_github_token() == "stored-gh-token"
+
+ mock_run.assert_called_once()
+ call_env = mock_run.call_args.kwargs["env"]
+ assert "GH_TOKEN" not in call_env
+ assert "GITHUB_TOKEN" not in call_env
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def test_retrieve_github_token_falls_back_to_env_token(mock_run, monkeypatch):
+ monkeypatch.setenv("GH_TOKEN", "env-gh-token")
+ monkeypatch.setenv("GITHUB_TOKEN", "env-github-token")
+ mock_run.return_value = _completed_process(returncode=1)
+
+ assert retrieve_github_token() == "env-gh-token"
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def
test_retrieve_github_token_falls_back_to_env_token_when_gh_is_missing(mock_run,
monkeypatch):
+ monkeypatch.setenv("GITHUB_TOKEN", "env-github-token")
+ mock_run.side_effect = FileNotFoundError
+
+ assert retrieve_github_token() == "env-github-token"
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def test_retrieve_github_token_keeps_explicit_token(mock_run, monkeypatch):
+ monkeypatch.setenv("GITHUB_TOKEN", "env-token")
+
+ assert retrieve_github_token("explicit-token") == "explicit-token"
+
+ mock_run.assert_not_called()
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def
test_retrieve_github_token_does_not_treat_env_token_argument_as_explicit(mock_run,
monkeypatch):
+ monkeypatch.setenv("GITHUB_TOKEN", "env-token")
+ mock_run.return_value = _completed_process(returncode=0,
stdout="stored-gh-token\n")
+
+ assert retrieve_github_token("env-token") == "stored-gh-token"
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def
test_run_gh_command_retries_with_original_env_after_clean_env_failure(mock_run,
monkeypatch):
+ monkeypatch.setenv("GH_TOKEN", "env-gh-token")
+ monkeypatch.setenv("GITHUB_TOKEN", "env-github-token")
+ mock_run.side_effect = [
+ _completed_process(returncode=1),
+ _completed_process(returncode=0),
+ ]
+
+ result = run_gh_command(["gh", "workflow", "run", "docs.yml"],
capture_output=True)
+
+ assert result.returncode == 0
+ assert mock_run.call_count == 2
+ first_env = mock_run.call_args_list[0].kwargs["env"]
+ second_env = mock_run.call_args_list[1].kwargs["env"]
+ assert "GH_TOKEN" not in first_env
+ assert "GITHUB_TOKEN" not in first_env
+ assert second_env["GH_TOKEN"] == "env-gh-token"
+ assert second_env["GITHUB_TOKEN"] == "env-github-token"
+
+
[email protected]("airflow_breeze.utils.github.subprocess.run")
+def test_run_gh_command_does_not_retry_after_clean_env_success(mock_run,
monkeypatch):
+ monkeypatch.setenv("GITHUB_TOKEN", "env-token")
+ mock_run.return_value = _completed_process(returncode=0)
+
+ result = run_gh_command(["gh", "api", "repos/apache/airflow"],
capture_output=True)
+
+ assert result.returncode == 0
+ mock_run.assert_called_once()
Review Comment:
Coverage of the precedence happy paths is good. A few gaps worth adding:
- `run_gh_command(..., check=True)` when the clean-env attempt fails and
there's no env token to retry with — `result.check_returncode()` should raise
`CalledProcessError`. Not currently tested.
- `run_gh_command` with no `gh` on PATH (`subprocess.run` raises
`FileNotFoundError`). Unlike `retrieve_github_token`, the helper doesn't catch
it, so callers see the bare exception. Confirm that's intentional and pin it
with a test.
- `gh auth token` returning `returncode=0` with whitespace-only stdout —
should fall through to env token; currently uncovered.
---
Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before posting
##########
dev/breeze/src/airflow_breeze/utils/gh_workflow_utils.py:
##########
@@ -50,7 +50,7 @@ def tigger_workflow(workflow_name: str, repo: str, branch:
str = "main", **kwarg
command.extend(["-f", f"{key}={value}"])
console_print(f"[blue]Running command: {' '.join(command)}[/blue]")
- result = run_command(command, capture_output=True, check=False)
+ result = run_gh_command(command, capture_output=True)
Review Comment:
Replacing `run_command` with `run_gh_command` here means `--dry-run` no
longer skips this `gh workflow run ...` call — `run_gh_command` invokes
`subprocess.run(...)` directly and bypasses Breeze's dry-run integration.
Since this is a write operation, `breeze --dry-run workflow-run ...` will
now actually trigger the workflow. Worth either threading `get_dry_run()` into
`run_gh_command` or explicitly documenting the behavior change.
Same mechanism applies to the read-only calls in `get_workflow_run_id`,
`get_workflow_run_info`, and `workflow_commands.py`; lower impact there, but
the loss of verbose-mode command echoing is also worth noting.
---
Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before posting
##########
dev/breeze/src/airflow_breeze/commands/release_management_commands.py:
##########
@@ -3062,17 +3054,7 @@ def generate_issue_content_core(
def _get_github_token(github_token: str) -> str:
"""Return github_token as-is, or fall back to ``gh auth token``."""
- if github_token:
- return github_token
- result = run_command(
- ["gh", "auth", "token"],
- capture_output=True,
- text=True,
- check=False,
- )
- if result.returncode == 0:
- return result.stdout.strip()
- return github_token
+ return retrieve_github_token(github_token) or github_token
Review Comment:
Two small things:
1. `retrieve_github_token` already does `return token or env_token`, so the
trailing `or github_token` only kicks in when `github_token == ""` *and* env
resolution fails — same thing it would fall back to anyway. Effectively dead
code.
2. Annotation says `-> str`, but on the empty-token / no-env / no-`gh-auth`
path this returns `""` (or `None` if the wrapper is removed). Either tighten
the annotation to `str | None` or drop the wrapper and call
`retrieve_github_token(github_token) or ""` at the call site.
---
Drafted-by: Claude Opus 4.7 (1M context); reviewed by @potiuk before posting
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
