sadpandajoe commented on code in PR #43502: URL: https://github.com/apache/superset/pull/43502#discussion_r3868792646
########## requirements/development.in: ########## @@ -16,5 +16,8 @@ # specific language governing permissions and limitations # under the License. # --e .[development,bigquery,druid,duckdb,fastmcp,gevent,gsheets,mysql,postgres,presto,prophet,trino,thumbnails] +-e .[development,bigquery,cockroachdb,crate,db2,druid,duckdb,elasticsearch,fastmcp,gevent,gsheets,mssql,mysql,oracle,postgres,presto,prophet,trino,thumbnails] Review Comment: This puts the DB2 stack in every development install, including the multi-platform dev Docker build. `ibm-db` has no Linux arm64 wheel, so the arm64 build now falls back to an unsupported source install and fails before tests run. Could the CI-only DB2 dependency be scoped out of the baseline dev requirements (or platform-gated)? ########## tests/testcontainers/db_engine_specs/test_cockroachdb.py: ########## @@ -0,0 +1,98 @@ +# Licensed to the Apache Software Foundation (ASF) under one +# or more contributor license agreements. See the NOTICE file +# distributed with this work for additional information +# regarding copyright ownership. The ASF licenses this file +# to you under the Apache License, Version 2.0 (the +# "License"); you may not use this file except in compliance +# with the License. You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, +# software distributed under the License is distributed on an +# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +# KIND, either express or implied. See the License for the +# specific language governing permissions and limitations +# under the License. +""" +Tests db_engine_specs.cockroachdb against a real CockroachDB instance, +spun up on demand via testcontainers. Run nightly (see +.github/workflows/nightly-testcontainers.yml), not on every merge -- these Review Comment: This points to `nightly-testcontainers.yml`, which is not part of the current tree; the workflow added here is `testcontainers.yml`. The same stale path is in the Crate and Trino module docstrings, so contributors cannot follow the test-run guidance. Could these references use the current workflow name? ########## superset/db_engine_specs/cockroachdb.py: ########## @@ -44,7 +44,7 @@ class CockroachDbEngineSpec(PostgresEngineSpec): DatabaseCategory.TRADITIONAL_RDBMS, DatabaseCategory.OPEN_SOURCE, ], - "pypi_packages": ["cockroachdb"], + "pypi_packages": ["sqlalchemy-cockroachdb"], Review Comment: This metadata is used as the required-package guidance, but the plain `cockroachdb://` URL also needs a DBAPI and `sqlalchemy-cockroachdb` does not install one. A manual installation following this list still cannot connect, even though the extra now includes `psycopg2-binary`. Could this list include the supported DBAPI too? -- 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]
