CLI: Setup Azure missing parameter during setup/export - #5220
CLI: Setup Azure missing parameter during setup/export#5220MonkeyCanCode wants to merge 5 commits into
Conversation
github-project-automation
Bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
flyrain
left a comment
flyrain
left a comment
There was a problem hiding this comment.
LGTM. Left minor comments.
| # The URL for OAuth consent. | ||
| consent_url: "https://login.microsoftonline.com/consent" | ||
| # Set to true if the Azure storage account has a hierarchical namespace enabled. | ||
| # This scopes SAS tokens down to the most specific path. |
There was a problem hiding this comment.
The comment explains the upside of true but not the failure mode of guessing. Per configuring-azure-blob-cloud-storage-specific.md:71-73, this value has to match whether HNS is actually enabled on the account, if they disagree, tokens are scoped against directory ACLs that don't exist and you get runtime 403s. Worth one more clause: "must match the storage account's actual HNS setting."
There was a problem hiding this comment.
Thanks for chiming in. +1 on the doc change.
There was a problem hiding this comment.
Made a change around this. Please take a look.
flyingImer
left a comment
flyingImer
left a comment
There was a problem hiding this comment.
The fix is right and I'd merge it. Losing hierarchical is not cosmetic. Just some minor inline
| "tenant_id", | ||
| "multi_tenant_app_name", | ||
| "consent_url", | ||
| "hierarchical", |
There was a problem hiding this comment.
IIUC this allowlist is more than a passthrough: since CatalogsCommand's post_init turns a missing hierarchical into an explicit False rather than leaving it unset, wouldn't a field left off this list silently widen an HNS catalog's SAS scope back to container-wide instead of just failing to round-trip? Might be worth a short comment here flagging that an omission isn't just a data-loss bug, it can change a security default.
There was a problem hiding this comment.
A missing hierarchical flag is treated as false, IIRC.
4 participants
Similar to #5219 but for Azure. This PR adds the missing field (hierarchical) that got loss during setup apply/export.
Checklist
CHANGELOG.md(if needed)site/content/in-dev/unreleased(if needed)