Repository navigation
Conversation
boring-cyborg
Bot
added
area:providers
kind:documentation
provider:microsoft-azure
|
I added a note in the PR about enhancing the docs. I think a couple of things that I'd like to know if I were a user setting this up from scratch.
|
|
These changes offer a nice starting point: https://github.com/apache/airflow/pull/66993/changes |
Thanks for the review. I'll add provider-level bundle docs for WASB following the pattern in #66993 (providers/microsoft/azure/docs/bundles/index.rst, Guides entry, cross-link to core Dag bundles), and cover auth, managed-identity permissions, container/prefix setup, networking, and reusing the same wasb Connection as in Dags. I'll push an update shortly. |
yuseok89
left a comment
yuseok89
left a comment
There was a problem hiding this comment.
Left a couple of small comments inline.
Please take a look when you have a moment.
|
@Nishieee — There are 4 unresolved review thread(s) on this PR from @dominikhei, @jroachgolf84, @yuseok89. Could you either push a fix or reply in each thread explaining why the feedback doesn't apply? Once you believe the feedback is addressed, mark the thread as resolved so the reviewer isn't re-pinged needlessly. Thanks! 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. |
0144712 to
a16419b
Compare
|
@Nishieee — There are 2 unresolved review thread(s) on this PR, and you have engaged with each one (post-review commits and/or in-thread replies). Could you confirm whether you believe the feedback is fully addressed and the PR is ready for maintainer review confirmation? If yes, reply here (a short "yes / ready" is fine) and an Apache Airflow maintainer will pick the PR up from the review queue on the next sweep. If you are still working on a thread, please reply with what is outstanding so the threads stay unresolved on purpose. 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 |
yes |
jroachgolf84
left a comment
jroachgolf84
left a comment
There was a problem hiding this comment.
Nice work, all the changes LGTM. I'm not a maintainer, so my review doesn't count towards your required approvals. But nice job!
potiuk
added
the
ready for maintainer review
|
@eladkal - can you take a look at this one? I know that you were involved on the initial issue. |
potiuk
removed
the
ready for maintainer review
2d3b4e3 to
51d73e9
Compare
|
Probably needs 2nd eye from someone more familiar witht he bundle area |
jroachgolf84
left a comment
jroachgolf84
left a comment
There was a problem hiding this comment.
My docs issues are addressed, thanks!
aaron-y-chen
left a comment
aaron-y-chen
left a comment
There was a problem hiding this comment.
Thanks for the contribution :)
dabla
left a comment
dabla
left a comment
There was a problem hiding this comment.
Solid addition overall: WasbDagBundle mirrors the S3DagBundle/GCSDagBundle structure, check_for_container now uses ContainerClient.exists(), the "Deleting …" log wording and the prefix + "/" anchoring from the previous rounds are in, and the hook and bundle tests are otherwise thorough. Every earlier thread is either addressed in the diff or was explicitly closed by discussion (structlog positional args, prefix="" default, _view_url_template, LoggingMixin deferred to #69859), with one exception below.
Two things block the merge:
- SAS token in the view URL (comment [3]). The response to @aaron-y-chen's thread fixed the URL shape but kept the query string, so a SAS-based connection leaks its signature into the link shown to every UI user. Dropping
query/fragmentinurlunsplitis a one-line fix; a test with a?sig=URL should accompany it. - No path-traversal guard in
sync_to_local_dir(comment [4]). S3 gained one in #67847 after this PR was opened and GCS has one too; the WASB port needs the same check plus the matching test.
Also worth doing before merge: prefix-path test coverage for the hook (comment [5]), and the two nits on the unanchored prefix existence check and the missing docs heading underline.
This was a diff-based pass; I compared against the S3 and GCS hooks and the bundle manager in the repo, but did not trace beyond that.
…e/bundles/wasb.py Co-authored-by: Yuseok Jo <yuseok89@gmail.com>
The debug logs claimed a file or directory had been deleted before item.unlink()/os.rmdir() actually ran, so a failed deletion would still be logged as successful. Co-authored-by: Cursor <cursoragent@cursor.com>
The previous commit changed the stale-file/dir deletion log messages from past to present tense but missed updating this test's assertions, breaking CI across every provider compatibility matrix job. Co-authored-by: Cursor <cursoragent@cursor.com>
blob_service_client.url includes the SAS query string when the connection is authenticated via a full SAS URL. Appending the container name and prefix as plain string suffixes placed them after the query string instead of in the path, producing a broken link. Build the URL by rejoining the path and query components instead.
…s directory A SAS-authenticated connection put its signature into the bundle view URL shown to every UI user, and a crafted blob name could be written outside the bundle directory on the Dag processor host. The prefix existence check also accepted sibling prefixes, letting a bundle initialize and then sync nothing.
Unspecced mocks accept any attribute, so the sync tests would keep passing even if the hook started reading a BlobProperties field that does not exist.
a30ff7d to
ac68f33
Compare
The previous commit rewrote uv.lock with an older uv, which lowered the lock revision and dropped the exclude-newer options. This PR adds no dependency, so the lock file matches main again.
This branch has not been deployed
8 participants
Summary
Adds a Dag bundle for Azure Blob Storage so Dags can be loaded from a container (with optional prefix), similar to
S3DagBundleandGCSDagBundle. IntroducesWasbDagBundle, extendsWasbHookwith container checks andsync_to_local_dir, registers the bundle in provider metadata, documents it indag-bundles.rst, and adds unit tests.Manual verification: Tested with Breeze against a real Azure storage account:
wasbconnection,dag_processor.dag_bundle_config_listpointing atWasbDagBundle, Dag parsed from blob and visible in the UI. PR includes screenshots (Azure container + Airflow Dags list).closes: #66987
Was generative AI tooling used to co-author this PR?
Generated-by: Cursor following the guidelines
Important
🛠️ Maintainer triage note for @Nishieee · by
@potiuk· 2026-07-08 15:51 UTCSome review feedback from
@eladkalis waiting on you (4 unresolved threads):The ball is in your court — you've been assigned to this PR. Reply or push a fix in each thread, then mark them resolved. See the Pull Request quality criteria.
Automated triage — may be imperfect; a maintainer takes the next look.