villebro commented on issue #434: URL: https://github.com/apache/superset-kubernetes-operator/issues/434#issuecomment-6171955457
Some more thoughts: - **Mark managed objects as externally managed:** Superset databases and datasets have `is_managed_externally`/`external_url` fields, and the v1 importer accepts both. If we set them, with `external_url` pointing at the CR or its Git source, the UI hides edit controls. Upstream `master` now also refuses API edits to managed datasets. Without this, edits made in the UI get silently overwritten on the next sync. - **Conventions for the first non-`Superset` CRD:** These will probably set the pattern for later dedicated CRs, like the approval CRs discussed in #314, so let's be deliberate about the `supersetRef` shape, condition types and finalizer behavior. `supersetRef` should be same-namespace only. Otherwise anyone who can create these CRs could attach connections to a Superset in another namespace. Two CRs claiming the same UUID or name on one instance should report a conflict condition rather than overwrite each other on every sync. - **Serialize and batch imports per instance:** Each import Job boots a full Superset app. An Argo CD sync that applies 50 datasets shouldn't mean 50 pods racing on the same metastore. One catch: Superset runs each import bundle in a single transaction, so a batch fails as a unit, and reporting the failure on the right CR needs some thought. - **Connection surface:** Not every engine fits host/port/username/password. BigQuery and Snowflake key-pair auth use `encrypted_extra`, and `ssh_tunnel` carries its own password and private key. Those need Secret-backed counterparts too. Security-relevant flags (`allow_dml`, file upload, `expose_in_sqllab`, `impersonate_user`) should be explicit typed fields with safe defaults, not an untyped passthrough. Engines whose driver isn't in the stock image should fail with a clear condition. - **Superset version range:** Users pick the Superset image, so the generated import bundle has to work with every Superset version we support. We should define a minimum and only emit the fields the user actually set. - **Testing:** Our e2e suite currently stubs the lifecycle commands and never runs a real Superset. Most of this feature's correctness depends on Superset's importer (UUID-based updates, managed-externally handling), so it probably needs an e2e that imports into a real Superset and Postgres. However, this can probably be deferred to a later phase, and is probably not something we'd want to run on each PR. -- 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]
