Ujjwaljain16 commented on PR #44142: URL: https://github.com/apache/superset/pull/44142#issuecomment-5693830204
@gabotorresruiz thanks for both passes on this — the head-conflict catch and the write-permission question in particular were exactly the kind of thing worth slowing down for. Here's everything that's landed since: **The Alembic head conflict** — fixed with a merge migration reconciling all three heads, like you suggested. Then master moved again and brought in its own fix for the same underlying split (a different merge migration resolving the same two heads), which made mine redundant and would've quietly created a new two-way split underneath it. Caught that by actually computing the head graph from all the migration files rather than trusting `git status`, dropped my old merge migration, and added a smaller one reconciling just master's current head with ours. Single head confirmed end to end with a real `superset db upgrade` run each time, not just eyeballing the chain. On the write-permission question — you were right, and chasing it further turned up more of the same. `can_testconn`, `can_sqllab_viz`, and `can_import_dashboards` all move to the delete migration now instead of resurrecting a broader live permission, exactly as you flagged. While I was in there I found a fourth one you hadn't mentioned, `can_add_slices` → `Chart.can_write`, with the same problem plus what looks like a wrong-resource mapping on top (the old action mutated a dashboard's chart list, not chart records themselves) — moved that too. `can_copy_dash` is the one I kept as a rename, since Superset's actual `copy_dash` endpoint today is still gated by that exact permission — a real like-for-like match rather than accidental broadening, not just the same pattern as the others. Also trimmed both migration docstrings per your nit — they don't narrate the earlier 25-entry/11-entry draft history anymore, just the final evidence. And added the `UPDATING.md` entry you suggested, explaining the delete-vs-rename split for anyone with custom roles. One more thing came out of an automated review pass afterward, which I checked by hand before touching anything: two real bugs in the tests from the previous round. The `PVM_LIST`/`PVM_MAP` disjointness check was comparing against a dict key (`"Superset"`) that never actually exists in `NEW_PVMS`, so it passed no matter what was in the list — fixed to flatten every successor permission across all views and check against that instead. And the idempotency test's "is it really gone" check only matched on permission name, which would've spuriously failed if the dict's first entry ever happened to be one of the few same-name-different-view pairs already in the map (`can_recent_activity` on Superset vs. `Log`, for instance) — now it filters on both. Neither touched any production migration logic. Tests are at 161 passing, ruff's clean, and I re-ran the full chain fresh after every change rather than assuming it still worked. -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
