jason810496 commented on code in PR #54812:
URL: https://github.com/apache/airflow/pull/54812#discussion_r2544362533
##########
providers/github/src/airflow/providers/github/hooks/github.py:
##########
@@ -55,17 +54,33 @@ def get_conn(self) -> GithubClient:
conn = self.get_connection(self.github_conn_id)
access_token = conn.password
host = conn.host
-
- # Currently the only method of authenticating to GitHub in Airflow is
via a token. This is not the
- # only means available, but raising an exception to enforce this
method for now.
- # TODO: When/If other auth methods are implemented this exception
should be removed/modified.
- if not access_token:
- raise AirflowException("An access token is required to
authenticate to GitHub.")
+ extras = conn.extra_dejson or {}
+
+ if access_token:
+ auth: Auth.Auth = Auth.Token(access_token)
+ elif extras:
+ if key_path := extras.get("key_path"):
+ if not key_path.endswith(".pem"):
+ raise RuntimeError("Unrecognised key file")
Review Comment:
```suggestion
raise RuntimeError("Unrecognised key file: expected a
.pem private key")
```
##########
providers/github/src/airflow/providers/github/hooks/github.py:
##########
@@ -55,17 +54,33 @@ def get_conn(self) -> GithubClient:
conn = self.get_connection(self.github_conn_id)
access_token = conn.password
host = conn.host
-
- # Currently the only method of authenticating to GitHub in Airflow is
via a token. This is not the
- # only means available, but raising an exception to enforce this
method for now.
- # TODO: When/If other auth methods are implemented this exception
should be removed/modified.
- if not access_token:
- raise AirflowException("An access token is required to
authenticate to GitHub.")
+ extras = conn.extra_dejson or {}
+
+ if access_token:
+ auth: Auth.Auth = Auth.Token(access_token)
+ elif extras:
+ if key_path := extras.get("key_path"):
+ if not key_path.endswith(".pem"):
+ raise RuntimeError("Unrecognised key file")
+ with open(key_path) as key_file:
+ private_key = key_file.read()
+
+ app_id = extras.get("app_id")
+ installation_id = extras.get("installation_id")
+ if not isinstance(installation_id, int):
+ raise RuntimeError("The provided installation_id should be
integer.")
+ if not isinstance(app_id, (str | int)):
+ raise RuntimeError("The provided app_id should be integer or
string.")
+ token_permissions = extras.get("token_permissions", None)
+
+ auth = Auth.AppAuth(app_id,
private_key).get_installation_auth(installation_id, token_permissions)
Review Comment:
It seems it might raise variable not define error for `private_key`, as
`private_key` is only defined in `if key_path := extras.get("key_path"):`
branch. The error occur if there isn't `key_path` in extra.
##########
providers/github/docs/connections/github.rst:
##########
@@ -40,3 +40,27 @@ Host (optional)
.. code-block::
https://{hostname}/api/v3
+
Review Comment:
Just to make double check that we support GitHub Authentication by either
1. `access_token`
2. `extras.get("key_path")` (SSH Private Key)
If this is the case, how about mentioning the second approach for
authentication?
Since we only mention the first approach above. Or maybe we can change the
`Extra Parameters` section name to something like `Authenticate with Private
Key` ?
##########
providers/github/src/airflow/providers/github/hooks/github.py:
##########
@@ -55,17 +54,33 @@ def get_conn(self) -> GithubClient:
conn = self.get_connection(self.github_conn_id)
access_token = conn.password
host = conn.host
-
- # Currently the only method of authenticating to GitHub in Airflow is
via a token. This is not the
- # only means available, but raising an exception to enforce this
method for now.
- # TODO: When/If other auth methods are implemented this exception
should be removed/modified.
- if not access_token:
- raise AirflowException("An access token is required to
authenticate to GitHub.")
+ extras = conn.extra_dejson or {}
+
+ if access_token:
+ auth: Auth.Auth = Auth.Token(access_token)
+ elif extras:
+ if key_path := extras.get("key_path"):
+ if not key_path.endswith(".pem"):
+ raise RuntimeError("Unrecognised key file")
Review Comment:
Would it be better to replace all the `RuntimeError`with `ValueError` for
validating the attributes?
--
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]
