jedcunningham commented on code in PR #46181:
URL: https://github.com/apache/airflow/pull/46181#discussion_r1932562511
##########
airflow/dag_processing/bundles/git.py:
##########
@@ -60,6 +61,7 @@ def get_ui_field_behaviour(cls) -> dict[str, Any]:
"extra": json.dumps(
{
"key_file": "optional/path/to/keyfile",
+ "private_key": "optional/private_key",
Review Comment:
nit: maybe say "optional inline private key", just don't make it look like a
path basically.
##########
airflow/dag_processing/bundles/git.py:
##########
@@ -84,6 +94,13 @@ def _process_git_auth_url(self):
elif not self.repo_url.startswith("git@") or not
self.repo_url.startswith("https://";):
self.repo_url = os.path.expanduser(self.repo_url)
+ def _setup_inline_key(self):
+ with tempfile.NamedTemporaryFile(mode="w", delete=False) as
tmp_keyfile:
Review Comment:
We might want to refactor things a bit.
- We shouldn't leave this fine around imo, so let's do a context manager so
we can clean up.
- This likely means the way we do env likely needs to be adjusted a bit too.
- This also means we won't have to write the pk out when creating a bundle
just to do the view_url (e.g. on the webserver).
##########
tests/dag_processing/test_dag_bundles.py:
##########
@@ -160,6 +184,16 @@ def test_correct_repo_urls(self, conn_id,
expected_repo_url):
hook = GitHook(git_conn_id=conn_id)
assert hook.repo_url == expected_repo_url
+ def test_given_both_private_key_and_key_file(self):
+ with pytest.raises(
+ AirflowException, match="Both 'key_file' and 'private_key' cannot
be provided at the same time"
+ ):
+ GitHook(git_conn_id=CONN_BOTH_PATH_INLINE)
+
+ def test_only_inline_connection_has_tmp_keyfile(self):
Review Comment:
We should beef up our coverage on this path too. "how" depends how we
refactor I think, but reach out if you need ideas :)
##########
airflow/dag_processing/bundles/git.py:
##########
@@ -70,10 +72,18 @@ def __init__(self, git_conn_id="git_default", *args,
**kwargs):
connection = self.get_connection(git_conn_id)
self.repo_url = connection.host
self.auth_token = connection.password
+ self.private_key = connection.extra_dejson.get("private_key")
self.key_file = connection.extra_dejson.get("key_file")
self.env: dict[str, str] = {}
+
+ if self.key_file and self.private_key:
+ raise AirflowException("Both 'key_file' and 'private_key' cannot
be provided at the same time")
+ if self.private_key:
+ self._setup_inline_key()
if self.key_file:
Review Comment:
```suggestion
if self.key_file or self.private_key:
```
right?
--
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]
