Add GitHub App authentication for git DAG bundles - #64422
Conversation
|
Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contributors' Guide (https://github.com/apache/airflow/blob/main/contributing-docs/README.rst)
|
0639a51 to
d080795
Compare
|
@RaphCodec Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
There was a problem hiding this comment.
Pull request overview
Adds GitHub App-based authentication support to the Git provider’s GitHook so Git-backed DagBundles can authenticate to GitHub without SSH deploy keys (using an installation access token instead).
Changes:
- Extend
GitHookconnection extras/UI placeholders to accept GitHub App identifiers and generate an installation token for HTTPS cloning. - Add an optional dependency extra for GitHub App support (
pygithub/PyGithub) in the git provider. - Update the workspace lockfile to include the new optional extra and reflect a refreshed dependency resolution.
Reviewed changes
Copilot reviewed 2 out of 3 changed files in this pull request and generated 6 comments.
| File | Description |
|---|---|
providers/git/src/airflow/providers/git/hooks/git.py |
Adds GitHub App auth fields and token generation logic to rewrite HTTPS repo URLs with an installation token. |
providers/git/pyproject.toml |
Introduces an optional dependency extra for GitHub App support. |
uv.lock |
Updates the lockfile to include the new optional extra and updated resolved packages. |
1abb9fb to
23cf74d
Compare
687fb39 to
533e92f
Compare
|
Thanks for the feedback. I went over the code more thourghly, improved it and updated the branch. I will try to update the branch at least once a day to prevent merge conflicts. |
27884e7 to
a2d5a9a
Compare
|
@RaphCodec Converting to draft — this PR doesn't yet meet our Pull Request quality criteria.
See the linked criteria for how to fix each item, then mark the PR "Ready for review". This is not a rejection — just an invitation to bring the PR up to standard. No rush. Note: This comment was drafted by an AI-assisted triage tool and may contain mistakes. Once you have addressed the points above, an Apache Airflow maintainer — a real person — will take the next look at your PR. We use this two-stage triage process so that our maintainers' limited time is spent where it matters most: the conversation with you. |
1a323e7 to
e7e490c
Compare
|
@potiuk I made the requested changes and address issues 1-9. |
8f52fd5 to
6f49aca
Compare
jscheffl
left a comment
There was a problem hiding this comment.
I am not an expert of GH App Auth, so I can just skim over the code. Does not look bad but I can not really judge. Some other maintainer around who known more?
0976f2c to
19aa746
Compare
6e4ad47 to
c127dfe
Compare
|
@potiuk I checked the failing provider distribution tests and they seem to be unrelated to the changes in my PR. The failing tests (https://github.com/RaphCodec/airflow/actions/runs/28319050045/job/83900082499) failed due to issues with the amazon and azure providers. Example Error: ================================================================================================= ERRORS =================================================================================================
_______________________________________________________ ERROR collecting providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_batch.py ________________________________________________________
ImportError while importing test module '/opt/airflow/providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_batch.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:
/usr/python/lib/python3.10/importlib/__init__.py:126: in import_module
return _bootstrap._gcd_import(name[level:], package, level)
providers/microsoft/azure/tests/unit/microsoft/azure/hooks/test_batch.py:24: in <module>
from azure.batch import BatchServiceClient, models as batch_models
E ImportError: cannot import name 'BatchServiceClient' from 'azure.batch' (/usr/python/lib/python3.10/site-packages/azure/batch/__init__.py)
_____________________________________________________ ERROR collecting providers/microsoft/azure/tests/unit/microsoft/azure/operators/test_batch.py ______________________________________________________
ImportError while importing test module '/opt/airflow/providers/microsoft/azure/tests/unit/microsoft/azure/operators/test_batch.py'.
Hint: make sure your test modules/packages have valid Python names.
Traceback:I'm not sure how those would be resolved in this PR as they are unrelated so I'd like to mark this PR for review. If you believe this is related to my PR please let me know and I'll look into this further. I ran all the static checks locally and they passed, and the CI images all built successfully. |
|
@potiuk I reviewed the provider distributions tests / Compat test logs from this PR. A lot of the errors look to be import errors or a faillure to I have not been able to resolve the failing tests, yet. However, it doesn't appear that the code in my PR is causing the errors as, so I'm marking this for review. I know that the Compat tests should pass to be in line with the PR quality standards, but I am not sure why the Compat tests are having import issues. If there is something that I am missing I'd be happy to fix it. I'll keep looking for a solution. |
|
provider distributions tests / Compat tests are passing now and CI images are building as of this run |
|
Hi, any updates on this PR? We are really looking forward to this feature being part of the next release. |
|
I'm just waiting for the pr to be reviewed. I have tried to address all the changes that were requested. I don't have any other changes for it as of now. I realized that I haven't rebased it in a bit, so I will rebase it later today. Probably in 8 hours or so. Is it possible to review it in the meantime as the CI checks passed? |
|
I updated my feature and main bracnhes and was able to get the CI checks to pass again. Some of these test failed becuase my main branch was not up to date with upstream. I was only rebasing my feature branch and as soon as I updated my main branch also the tests started to pass again. I didn't realize I needed to update both, so I apologize for the confusion on my part. Test runs: Some test that failed still are the Additional PROD e2e tests and the k8s tests. Those are failing becuase they are looking for images in my fork that don't exist. I assume that those should be fine once the maintainer approved workflow runs, but please correct me if I am wrong. The provdier wheel test seems to be failing for a similiar reason, but all of the provider distirbution tests are passing. The |
Miretpl
left a comment
There was a problem hiding this comment.
Similar to Jens review really - would appriciate other maintainer review
| if (self.github_app_id is not None and self.github_installation_id is None) or ( | ||
| self.github_app_id is None and self.github_installation_id is not None | ||
| ): |
There was a problem hiding this comment.
| if (self.github_app_id is not None and self.github_installation_id is None) or ( | |
| self.github_app_id is None and self.github_installation_id is not None | |
| ): | |
| if None in (self.github_app_id, self.github_installation_id): |
| # but also keep a dedicated attribute so configure_hook_env() can avoid | ||
| # treating the GitHub App PEM as an SSH key. | ||
| self.github_app_private_key = self.private_key | ||
| self.auth_token = "" |
There was a problem hiding this comment.
Shouldn't we require an empty password in the Airflow connection instead of silently setting this to an empty string?
|
|
||
| return " ".join(parts) | ||
|
|
||
| def _get_github_app_token(self): |
There was a problem hiding this comment.
Missing return annotation.
| @contextlib.contextmanager | ||
| def configure_hook_env(self): | ||
| if self.private_key: | ||
| self._ensure_github_app_token() |
There was a problem hiding this comment.
We could move it under if below and remove the check in _ensure_github_app_token as it would be duplicated.
| self.user_name, self.auth_token, self.github_app_token_exp = self._get_github_app_token() | ||
|
|
||
| @contextlib.contextmanager | ||
| def _github_app_askpass_env(self): |
There was a problem hiding this comment.
Missing return annotation type.
| raise ValueError( | ||
| "Both 'github_app_id' and 'github_installation_id' must be provided to use GitHub App Authentication" | ||
| ) | ||
| if self.github_app_id is not None and self.github_installation_id is not None: |
There was a problem hiding this comment.
| if self.github_app_id is not None and self.github_installation_id is not None: | |
| else: |
| self.user_name, self.auth_token, self.github_app_token_exp = self._get_github_app_token() | ||
|
|
||
| @contextlib.contextmanager | ||
| def _github_app_askpass_env(self): |
There was a problem hiding this comment.
Can't we use the currently implemented _passphrase_askpass_env, but with a modification for askpass_script 🤔?
Purpose
Adding GitHub App Authentication to the Git Provider. This feature is intended to be a security benefit for those who want to use GitHub Dag Bundles in Airflow 3, but for whatever reason cannot use a SSH deploy key to authenticate to GitHub and would prefer not to use their own PAT.
In case of an existing issue, reference it using one of the following:
Was generative AI tooling used to co-author this PR?
{pr_number}.significant.rst, in airflow-core/newsfragments. You can add this file in a follow-up commit after the PR is created so you know the PR number.Important
🛠️ Maintainer triage note for @RaphCodec · by
@potiuk· 2026-07-08 15:38 UTCHelpful heads-up from the maintainers — please address before this PR can be reviewed:
Build info,Platform: AMD,Migration round-trip check(+2 more)). See the contributor guide.Go SDK tests,Java SDK tests). See the contributor guide.Java SDK docs). See the contributor guide.MyPy providers checks). See the contributor guide.Full criteria: Pull Request quality criteria.
The ball is in your court — you've been assigned to this PR. Fix the above, then mark it Ready for review.
Automated triage — may be imperfect; a maintainer takes the next look.