jason810496 commented on code in PR #64422:
URL: https://github.com/apache/airflow/pull/64422#discussion_r3726676254
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -183,7 +216,80 @@ def _build_ssh_command(self, key_path: str | None = None)
-> str:
return " ".join(parts)
- def _process_git_auth_url(self):
+ def _get_github_app_token(self):
+ try:
+ from github import Auth, GithubIntegration
+ except ImportError as exc:
+ raise ImportError(
Review Comment:
```suggestion
raise AirflowOptionalProviderFeatureException(
```
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -183,7 +216,80 @@ def _build_ssh_command(self, key_path: str | None = None)
-> str:
return " ".join(parts)
- def _process_git_auth_url(self):
+ def _get_github_app_token(self):
+ try:
+ from github import Auth, GithubIntegration
+ except ImportError as exc:
+ raise ImportError(
+ "The PyGithub library is required for GitHub App
authentication. Please install it with 'pip install
apache-airflow-providers-git[github]'"
+ ) from exc
+
+ auth = Auth.AppAuth(self.github_app_id, self.github_app_private_key)
Review Comment:
Once `github_app_private_key` is dropped (see comment on `__init__`), this
can read `private_key` directly.
```suggestion
auth = Auth.AppAuth(self.github_app_id, self.private_key)
```
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -127,6 +141,25 @@ def __init__(
AirflowProviderDeprecationWarning,
stacklevel=2,
)
+ if None in (self.github_app_id, self.github_installation_id):
Review Comment:
`None in (self.github_app_id, self.github_installation_id)` is `True`
whenever *neither* field is set, so this raises for every non-GitHub-App
connection — including all pre-existing SSH/token connections that never set
these extras. This needs to gate on "is GitHub App auth being attempted" rather
than requiring both unconditionally, e.g.:
```python
github_app_fields = (self.github_app_id, self.github_installation_id)
if any(github_app_fields) and not all(github_app_fields):
raise ValueError(
"Both 'github_app_id' and 'github_installation_id' must be
provided to use GitHub App Authentication"
)
if all(github_app_fields):
if self.auth_token:
raise ValueError("Password field must be empty to use GitHub
App Auth")
if not (self.repo_url or "").startswith(("https://";, "http://";)):
raise ValueError(
f"GitHub App authentication requires an HTTPS repository
URL, but got: {self.repo_url!r}"
)
if self.key_file and not self.private_key:
with open(self.key_file, encoding="utf-8") as key_file:
self.private_key = key_file.read()
```
A couple of things folded in here:
- moved the cheap `auth_token`/URL checks ahead of the `key_file` filesystem
read, so a bad key path does not mask the clearer "password must be empty" error
- this also lets us drop `self.github_app_private_key` entirely (see
comments below) since `configure_hook_env` can disambiguate off
`github_app_id`/`github_installation_id` directly
##########
providers/git/src/airflow/providers/git/hooks/git.py:
##########
@@ -240,7 +346,15 @@ def _passphrase_askpass_env(self):
@contextlib.contextmanager
def configure_hook_env(self):
- if self.private_key:
+ if self.github_app_id is not None and self.github_installation_id is
not None:
+ self._ensure_github_app_token()
+ with self._github_app_askpass_env():
+ yield
+ return
+
+ # If a GitHub App PEM is present, it should not be treated as an SSH
key
+ # for configuring `GIT_SSH_COMMAND`.
+ if self.private_key and not self.github_app_private_key:
Review Comment:
Once `__init__` enforces "both GitHub App fields or neither" (see comment
above), this branch — the SSH-key path — is only ever reached when neither is
set, since the App case already `return`s above it. So `github_app_private_key`
never actually needs to exist: this can revert to the original `if
self.private_key:`, and the explanatory comments here and at the
`github_app_private_key` assignment in `__init__` can go too — they explain a
distinction that no longer exists once the attribute is removed.
--
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]
