Copilot commented on code in PR #7256:
URL: https://github.com/apache/ignite-3/pull/7256#discussion_r2641476061
##########
modules/platforms/python/tox.ini:
##########
@@ -14,17 +14,15 @@
# limitations under the License.
[tox]
-skipsdist = True
-envlist = codestyle,py{39,310,311,312,313}
+env_list = codestyle,py31{0,1,2,3,4}
+isolated_build = True
[testenv]
-passenv = TEAMCITY_VERSION IGNITE_HOME
-envdir = {homedir}/.virtualenvs/pyignite_dbapi_{envname}
+passenv = *
+use_develop = True
Review Comment:
The configuration key 'use_develop' is deprecated in favor of 'package =
editable' in tox 4.x. If using tox 4+, this should be updated. Additionally,
'isolated_build' on line 18 is only needed for PEP 517/518 builds and may
conflict with 'use_develop'.
##########
modules/platforms/python/tox.ini:
##########
@@ -14,17 +14,15 @@
# limitations under the License.
[tox]
-skipsdist = True
-envlist = codestyle,py{39,310,311,312,313}
+env_list = codestyle,py31{0,1,2,3,4}
+isolated_build = True
[testenv]
-passenv = TEAMCITY_VERSION IGNITE_HOME
-envdir = {homedir}/.virtualenvs/pyignite_dbapi_{envname}
+passenv = *
+use_develop = True
deps =
-r ./requirements/install.txt
-r ./requirements/tests.txt
recreate = True
-usedevelop = True
-commands =
- pytest {env:PYTESTARGS:} {posargs}
+commands = pytest -s --teamcity tests
Review Comment:
The hardcoded pytest arguments '-s --teamcity tests' remove flexibility. The
previous implementation used '{env:PYTESTARGS:} {posargs}' which allowed
customization via environment variables and command-line arguments. This change
removes the ability to customize pytest behavior without modifying the tox.ini
file.
```suggestion
commands = pytest {env:PYTESTARGS:- -s --teamcity} {posargs:tests}
```
##########
.github/workflows/python_dbapi_wheels.yml:
##########
@@ -13,15 +13,17 @@ jobs:
runs-on: ubuntu-latest
steps:
- - uses: actions/checkout@v4
+ - uses: actions/checkout@v6
with:
sparse-checkout: |
modules/platforms/
- - uses: actions/setup-python@v5
+ - uses: actions/setup-python@v6
+ with:
+ python-version: 3.11
- name: Install setuptools
- run: python -m pip install setuptools
+ run: python -m pip install --upgrade setuptools packaging>=24.2
Review Comment:
The version constraint 'packaging>=24.2' may be too strict. Version 24.2 was
released in November 2024. Consider whether this minimum version is necessary
or if a lower version would suffice for broader compatibility.
```suggestion
run: python -m pip install --upgrade setuptools packaging
```
##########
.github/workflows/python_dbapi_wheels.yml:
##########
@@ -40,46 +42,34 @@ jobs:
runs-on: ${{ matrix.os }}
strategy:
matrix:
- # macos-13 is an intel runner, macos-14 is apple silicon
- os: [ubuntu-22.04, ubuntu-24.04-arm, windows-2022, macos-13, macos-14]
+ include:
+ - os: macos-latest
+ arch: arm64
+ macos_deployment_target: "14.0"
+
+ - os: macos-15-intel
+ arch: x86_64
+ macos_deployment_target: "15.0"
Review Comment:
The runner 'macos-15-intel' may not exist. GitHub Actions currently provides
'macos-13' for Intel-based macOS runners. The 'macos-15' runner is Apple
Silicon (ARM64) based. Verify that 'macos-15-intel' is a valid runner label, or
change to 'macos-13' for Intel architecture.
```suggestion
- os: macos-13
arch: x86_64
macos_deployment_target: "13.0"
```
##########
.github/workflows/python_dbapi_wheels.yml:
##########
@@ -40,46 +42,34 @@ jobs:
runs-on: ${{ matrix.os }}
strategy:
matrix:
- # macos-13 is an intel runner, macos-14 is apple silicon
- os: [ubuntu-22.04, ubuntu-24.04-arm, windows-2022, macos-13, macos-14]
+ include:
+ - os: macos-latest
+ arch: arm64
+ macos_deployment_target: "14.0"
+
+ - os: macos-15-intel
+ arch: x86_64
+ macos_deployment_target: "15.0"
Review Comment:
The MACOSX_DEPLOYMENT_TARGET of "15.0" may be problematic. macOS 15
(Sequoia) was released in September 2024, but setting the deployment target to
15.0 may limit compatibility. Consider whether this high deployment target is
intentional or if a lower value like "13.0" or "14.0" would provide better
compatibility with more user systems.
```suggestion
macos_deployment_target: "14.0"
```
##########
modules/platforms/python/tox.ini:
##########
@@ -14,17 +14,15 @@
# limitations under the License.
[tox]
-skipsdist = True
-envlist = codestyle,py{39,310,311,312,313}
+env_list = codestyle,py31{0,1,2,3,4}
+isolated_build = True
[testenv]
-passenv = TEAMCITY_VERSION IGNITE_HOME
-envdir = {homedir}/.virtualenvs/pyignite_dbapi_{envname}
+passenv = *
Review Comment:
The change to `passenv = *` in the tox configuration causes all environment
variables from the CI or local shell (including secrets like API tokens,
credentials, and signing keys) to be injected into the test environment. If any
test code, plugin, or dependency is compromised, it can now directly read and
exfiltrate these secrets via `os.environ`, which was previously mitigated by
only passing specific variables (`TEAMCITY_VERSION`, `IGNITE_HOME`). Restrict
`passenv` to a minimal, explicit set of non-sensitive variables required for
the tests instead of using a wildcard.
```suggestion
passenv = TEAMCITY_VERSION IGNITE_HOME
```
--
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]
