git dag bundle to use the private key in submodule pull - #62938
Conversation
|
Oh; I saw the diff but I was not 100% sure it was the newline! |
|
@Pandry 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. |
|
Hi, I just rebased on main, but the build docs link you provided is a 404 (also I did not touch the documentation) |
|
Hi, I didn't hear back from my last comment |
There was a problem hiding this comment.
Pull request overview
Updates GitDagBundle submodule initialization to reuse the same SSH private-key configuration used when fetching the main DAG bundle, fixing pulls for private git submodules.
Changes:
- Wrap
git submodule sync/updatewith acustom_environment(GIT_SSH_COMMAND=...)context whenGitHookprovidesGIT_SSH_COMMAND. - Align submodule-fetch authentication behavior with
_fetch_bare_repo.
|
@Pandry Thanks for your earlier replies — some threads are still marked unresolved and there has been no movement for 20 days. 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. |
|
Quick follow-up to the triage comment above — one clarification on the "Unresolved review comments" item: Once you believe a thread has been addressed — whether by pushing a fix, or by replying in-thread with an explanation of why the suggestion doesn't apply — please mark the thread as resolved yourself by clicking the "Resolve conversation" button at the bottom of each thread. Reviewers don't auto-close their own threads, so an addressed-but-unresolved thread reads as "still waiting on the author" and keeps the PR from moving forward. The author doing the resolve-click is the expected convention on this project. 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. |
1ca3260 to
6a853e1
Compare
|
Added tests (and ensure they don't pass with the previous commit) and rebased from main |
|
There are still errors/ |
|
@Pandry A few things need addressing before review — see our Pull Request quality criteria. Issues found:
What to do next:
No rush — take your time. We appreciate your contribution and are happy to wait for updates. If you have questions, feel free to ask on the Airflow Slack. 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. |
|
@Pandry A few things need addressing before review — see our Pull Request quality criteria.
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. Drafted-by: Claude Code (Opus 4.7); reviewed by @potiuk before posting |
|
Yeah, sorry, I've been very sloppy with this, got a lot going on and it's not pressing because I added the patch as part of my image pipeline I was sure I tested everything and all tests passed (including the pre-commit) but I might have done that before the commit, so the pre-commit changed the files but I didn't commit them |
potiuk
left a comment
There was a problem hiding this comment.
Good fix, and sorry it sat this long. _fetch_bare_repo wraps its fetch in custom_environment(GIT_SSH_COMMAND=...) so the configured deploy key is used, but _fetch_submodules did not — so git submodule update ran without it and private submodules could not be pulled at all.
Copying the established pattern is the right call here rather than inventing a new one, and swapping self.bare_repo for self.repo is correct since submodules update in the working repo.
Since this has been open a while, I checked the pattern against current main rather than just against the branch: _fetch_bare_repo still uses exactly this nullcontext() / custom_environment shape, _fetch_submodules is still the unguarded two-liner, and nullcontext is already imported. So the fix still applies to the code as it stands today.
The tests cover both entry paths — initialize() with a pinned version and without, which routes through refresh() — and each asserts the exact call, so a regression would be caught rather than silently passing.
Rebased onto main before merging, as the branch had drifted a long way behind.
Drafted-by: Claude Code (Opus 5); reviewed by @potiuk before posting
The git dag bundle did not use the private key to pull submodules. This lead to issues when working with private submodules. This commit uses the same key used to pull the dag bundle to pull the submodules.
|
Awesome work, congrats on your first merged pull request! You are invited to check our Issue Tracker for additional contributions. |
The git dag bundle did not use the private key to
pull submodules.
This lead to issues when working with private submodules. This commit uses the same key used to pull the dag bundle to pull the submodules
I literally copy-pasted the code from
_fetch_bare_repo