You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@airflow.apache.org by "dstandish (via GitHub)" <gi...@apache.org> on 2025/04/15 03:05:15 UTC
[PR] Make git connection optional for git dag bundle [airflow]
dstandish opened a new pull request, #49270:
URL: https://github.com/apache/airflow/pull/49270
Make it so we don't need to define a git connection for a git dag bundle.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2043752197
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
I think we should not accept cases of both repo_url and git_conn_id.
From what I understand the repo_url means that we ignore the git_conn_id.
So both parameters should be mutually exclusive
Similar to:
https://github.com/apache/airflow/blob/243fe86d4b3e59bb12977b3e36ca3f2ed27ca0f8/airflow-core/src/airflow/secrets/local_filesystem.py#L203-L207
This is important because consider a case where user provided both, then someone went to Connection->Git and changed the connection name. This code will automatically change to use the `repo_url` possibly without intention to do so
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2043752197
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
I think we should not accept cases of both repo_url and git_conn_id.
From what I understand the repo_url means that we ignore the git_conn_id.
So both parameters should be mutually exclusive
Similar to:
https://github.com/apache/airflow/blob/243fe86d4b3e59bb12977b3e36ca3f2ed27ca0f8/airflow-core/src/airflow/secrets/local_filesystem.py#L203-L207
This is important because consider a case where user provided both, then someone went to Connection->Git and changed the connection name. This code will automatically change to use the `repo_url` without the knowledge of the dag author. It could be something that he did not intent.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "kaxil (via GitHub)" <gi...@apache.org>.
kaxil merged PR #49270:
URL: https://github.com/apache/airflow/pull/49270
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044083312
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
Left a comment: https://github.com/apache/airflow/pull/49270#pullrequestreview-2767486702
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "uranusjr (via GitHub)" <gi...@apache.org>.
uranusjr commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2043784615
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
The current behaviour is the other way around; `repo_url` is ignored if `git_conn_id` is provided.
If `repo_url` cannot be provided when loading from connection, that happens when a connection cannot be loaded? (The except block.)
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "ephraimbuddy (via GitHub)" <gi...@apache.org>.
ephraimbuddy commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044093050
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
Left a comment: https://github.com/apache/airflow/pull/49270#pullrequestreview-2767486702
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044514048
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
the "why would you supply both" thing...
the issue i envision is you have one git conn that you want to use for multiple things.
multiple bundles could use the same git conn, but not the other way around.
so therefore it seems reasonable to defer to the bundle config.
it's like when you have an operator, sometimes an option may be specified either at hook or operator, but usually if in both, you defer to operator, for the same reason.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "eladkal (via GitHub)" <gi...@apache.org>.
eladkal commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2043955626
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
Regardless, why would you supply both if only one considered? This isn't a fallback case... Its a mutal exclusive case isn't it?
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "ashb (via GitHub)" <gi...@apache.org>.
ashb commented on PR #49270:
URL: https://github.com/apache/airflow/pull/49270#issuecomment-2804472496
I'm not too familiar with the bundle backends, but having a separate class seems like an odd UX decision.
One of the reasons for wanting to make it optional is so that you can either:
- Point your git bundle at a public repo for easy demo/onboarding; or
- Make use of an existing SSH key on your machine.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044750734
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
Oh interesting ----- if you define a repo url in gitdagbundle, then it is passed to githook and used as repo url _instead_ of taken from host in the git hook.
so the precedence is as i indicated, it just was not obvious
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044506545
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
In my first iteration of this, i implemented this order of precedence:
* if repo_url defined in both, use the on in the bundle confige. that seems right to me.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Make git connection optional for git dag bundle [airflow]
Posted by "dstandish (via GitHub)" <gi...@apache.org>.
dstandish commented on code in PR #49270:
URL: https://github.com/apache/airflow/pull/49270#discussion_r2044813374
##########
providers/git/src/airflow/providers/git/bundles/git.py:
##########
@@ -79,16 +80,20 @@ def __init__(
)
self._log.debug("bundle configured")
- try:
- self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
- self.repo_url = self.hook.repo_url
- self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
- except AirflowException as e:
- self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ self.hook: GitHook | None = None
+ if self.git_conn_id:
+ try:
+ self.hook = GitHook(git_conn_id=self.git_conn_id, repo_url=self.repo_url)
+ except AirflowException as e:
+ self._log.warning("Could not create GitHook", conn_id=self.git_conn_id, exc=e)
+ else:
+ self.repo_url = self.hook.repo_url
+ self._log.debug("repo_url updated from hook", repo_url=self.repo_url)
Review Comment:
@eladkal
> This is important because consider a case where user provided both, then someone went to Connection->Git and changed the connection name. This code will automatically change to use the repo_url possibly without intention to do so
I share your concern.
This won't happen because the bundle config is respected over the hook. i added a test for this just now.
--
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: commits-unsubscribe@airflow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org