build: add test matrix for multiple python versions #144
Conversation
WalkthroughThe pull request modifies the GitHub Actions workflow in Changes
Sequence DiagramsequenceDiagram
participant GA as GitHub Actions
participant Build as build-python Job
participant Test as Run Python Unit Tests Job
GA->>Build: Trigger Workflow
Build->>Build: Checkout Repository
Build->>Build: Pull Dev Image
Build->>Build: Build Python Bindings
Build-->>GA: Upload Python Packages Artifact
Build->>Build: Export Python Versions
GA->>Test: Trigger Job
Test->>GA: Download Python Packages
Test->>Test: Run Unit Tests for Each Python Version
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
.github/workflows/test.yml (1)
16-19: Enhance input parameter documentationWhile the example is helpful, consider adding more detailed documentation about:
- Valid Python version formats (e.g., '3.11' vs '3.11.0')
- Supported Python version ranges
- Default behavior when parameter is empty
- test_python_versions: # e.g. "['3.11', '3.12', '3.13']" + test_python_versions: + # Example: "['3.11', '3.12', '3.13']" + # - Use semantic versioning format: '3.11', '3.12', etc. + # - Must be compatible with available Python versions + # - If empty, uses PYTHON_TEST_VERSION from Dockerfile
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
124-124: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
129-129: the runner of "docker/login-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[warning] 138-138: too many blank lines
(2 > 0) (empty-lines)
🔇 Additional comments (3)
.github/workflows/test.yml (3)
97-97: Well-implemented job conditions!The mutually exclusive conditions ensure that either the single-version or matrix job runs, but never both. This is an efficient implementation.
Also applies to: 117-117
114-137: Well-structured matrix testing implementationThe implementation effectively:
- Maintains backward compatibility with single-version testing
- Provides flexible multi-version testing through matrix strategy
- Handles failures appropriately with
fail-fast: falseThe core functionality is solid, with only minor improvements needed in documentation and action versions.
🧰 Tools
🪛 actionlint (1.7.4)
124-124: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
129-129: the runner of "docker/login-action@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
118-121: Add Python version compatibility validationWhile the matrix strategy is well-implemented, consider adding version validation to prevent failures with unsupported Python versions.
Consider adding a step to validate each version before running tests:
- name: Validate Python Version run: | if ! docker run --rm ${{ env.DOCKER_REGISTRY_PATH }}/${{ inputs.project_name }}-development:${{ inputs.project_version }} \ python${{ matrix.python-version }} --version &> /dev/null; then echo "Python ${{ matrix.python-version }} is not supported" exit 1 fiAlso applies to: 136-136
🧰 Tools
🪛 yamllint (1.35.1)
[error] 118-118: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (5)
.github/workflows/test.yml (5)
16-19: LGTM! Consider enhancing the input description.The input parameter is well-defined. Consider adding the default behavior to the description for better clarity.
- description: The versions of Python used in unit tests. + description: The versions of Python used in unit tests. If not provided, uses PYTHON_TEST_VERSION from the Dockerfile.
94-113: Consider using null coalescing for better readability.The conditional could be more explicit about handling undefined vs empty string cases.
- if: ${{ inputs.test_python_versions == '' }} + if: ${{ inputs.test_python_versions == null || inputs.test_python_versions == '' }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
118-118: Remove unnecessary comment markers.The
## !!markers appear to be unnecessary and should be removed.- image: openspacecollective/${{ inputs.project_name }}-development:${{ inputs.project_version }} ## !! + image: openspacecollective/${{ inputs.project_name }}-development:${{ inputs.project_version }}
135-158: Consider adding input validation for Python versions.While the matrix configuration is correct, it might be helpful to validate the input versions before using them.
if: ${{ inputs.test_python_versions != '' }} + # Add a pre-job to validate Python versions + jobs: + validate-versions: + runs-on: ubuntu-latest + steps: + - name: Validate Python versions + run: | + versions='${{ inputs.test_python_versions }}' + if ! echo "$versions" | jq -e 'type == "array" and all(type == "string")' >/dev/null; then + echo "Invalid Python versions format. Expected JSON array of strings." + exit 1 + fi🧰 Tools
🪛 yamllint (1.35.1)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 140-140: trailing spaces
(trailing-spaces)
94-158: Consider architectural documentation and cleanup strategy.The implementation provides good flexibility for Python version testing, but consider:
- Adding documentation in the workflow file explaining when to use container-based vs matrix-based testing
- Creating a timeline for removing the commented-out code once the new implementation is proven stable
🧰 Tools
🪛 actionlint (1.7.4)
128-128: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 140-140: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
128-128: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 139-139: trailing spaces
(trailing-spaces)
[error] 140-140: trailing spaces
(trailing-spaces)
[warning] 159-159: too many blank lines
(2 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push and Tag Base Image with Version / Build Development Image
🔇 Additional comments (1)
.github/workflows/test.yml (1)
114-134: Update action versions and fix formatting issues.Previous review comments about updating action versions and removing trailing spaces still apply.
🧰 Tools
🪛 actionlint (1.7.4)
128-128: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
16-19: Add input validation and improve documentation.While the input parameter is well-defined, consider these improvements:
- Add input validation to ensure valid JSON array format
- Document the supported Python versions
- Clarify the default behavior when the input is not provided
test_python_versions: # e.g. "['3.11', '3.12', '3.13']" description: The versions of Python used in unit tests. + # Supported versions: 3.8-3.12 + # If not provided, uses PYTHON_TEST_VERSION from Dockerfile + # Must be a valid JSON array string required: false type: string + default: ''
118-119: Remove commented line marker.The
## !!marker should be removed as it appears to be a leftover comment.- image: openspacecollective/${{ inputs.project_name }}-development:${{ inputs.project_version }} ## !! + image: openspacecollective/${{ inputs.project_name }}-development:${{ inputs.project_version }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 119-119: trailing spaces
(trailing-spaces)
127-128: Replace temporary git-lfs solution.The current git-lfs installation is marked as temporary. Consider:
- Including git-lfs in the base development image
- Document why git-lfs is needed
- - name: TMP install lfs - run: apt-get install -y --no-install-recommends git-lfs + - name: Install git-lfs for large file support + run: | + apt-get update + apt-get install -y --no-install-recommends git-lfs + git lfs install
137-160: Enhance matrix testing configuration.While the matrix testing setup is good, consider these improvements:
- Add test results collection and aggregation
- Consider adding test coverage reporting
- Add retry mechanism for flaky tests
- name: Run Unit Tests Python run: make test-unit-python-standalone ${{ format('test_python_version={0}', matrix.python-version) }} + - name: Upload Test Results + if: always() + uses: actions/upload-artifact@v4 + with: + name: python-test-results-${{ matrix.python-version }} + path: test-results/ + - name: Upload Coverage + uses: codecov/codecov-action@v3 + with: + flags: python-${{ matrix.python-version }}🧰 Tools
🪛 yamllint (1.35.1)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
130-130: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
[warning] 161-161: too many blank lines
(2 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push and Tag Base Image with Version / Build Development Image
🔇 Additional comments (2)
.github/workflows/test.yml (2)
134-135: Add error handling for ostk installation.The installation step should handle potential failures gracefully.
- - name: Try to install ostk - run: ostk-build && ostk-install + - name: Install ostk + run: | + if ! ostk-build; then + echo "Failed to build ostk" + exit 1 + fi + if ! ostk-install; then + echo "Failed to install ostk" + exit 1 + fi
Line range hint
1-161: Update action versions and fix formatting.
Action versions need updating for security and compatibility:
actions/checkout@v3→actions/checkout@v4docker/login-action@v2→docker/login-action@v3Clean up formatting:
- Remove trailing spaces on lines 116, 119, 126, 141, 142
- Remove extra blank lines at the end of file
- uses: actions/checkout@v3 + uses: actions/checkout@v4 - uses: docker/login-action@v2 + uses: docker/login-action@v3🧰 Tools
🪛 actionlint (1.7.4)
130-130: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
[warning] 161-161: too many blank lines
(2 > 0) (empty-lines)
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
134-135: 🛠️ Refactor suggestionAdd error handling for ostk installation.
The installation step should handle potential failures gracefully.
- - name: Try to install ostk - run: cd /app/build && ostk-build && ostk-install + - name: Install ostk + run: | + cd /app/build || exit 1 + if ! ostk-build; then + echo "Failed to build ostk" + exit 1 + fi + if ! ostk-install; then + echo "Failed to install ostk" + exit 1 + fi
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
16-19: LGTM! Consider enhancing the example comment.The input parameter is well-defined. Consider expanding the example comment to show a complete valid JSON string:
- test_python_versions: # e.g. "['3.11', '3.12', '3.13']" + test_python_versions: # e.g. '["3.11", "3.12", "3.13"]'
127-128: Remove temporary solution for git-lfs.The comment indicates this is a temporary solution. Consider adding git-lfs to the development image instead.
- - name: TMP install lfs - run: apt-get update && apt-get install -y --no-install-recommends git-lfs
94-113: Document the transition plan for Python test jobs.The workflow contains both commented and active test jobs. Please add documentation explaining:
- The transition plan from single-version to matrix testing
- The conditions for enabling matrix testing
- The timeline for removing the commented jobs
Consider adding this documentation in a comment block at the top of the file:
# Python Testing Strategy: # - Current: Container-based single version testing # - Future: Matrix-based multi-version testing (pending development image updates) # - Transition: Enabled via test_python_versions input parameterAlso applies to: 137-160
🧰 Tools
🪛 yamllint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
130-130: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
[warning] 161-161: too many blank lines
(2 > 0) (empty-lines)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push and Tag Base Image with Version / Build Development Image
🔇 Additional comments (2)
.github/workflows/test.yml (2)
Line range hint
1-161: Update action versions and fix formatting.
Action versions need updating for security and compatibility:
actions/checkout@v3→actions/checkout@v4docker/login-action@v2→docker/login-action@v3Clean up formatting:
- Remove trailing spaces on lines 113, 116, 119, 126, 141, 142
- Remove extra blank lines at the end of file
🧰 Tools
🪛 actionlint (1.7.4)
130-130: the runner of "actions/checkout@v3" action is too old to run on GitHub Actions. update the action's version to fix this issue
(action)
🪛 yamllint (1.35.1)
[error] 113-113: trailing spaces
(trailing-spaces)
[error] 116-116: trailing spaces
(trailing-spaces)
[error] 119-119: trailing spaces
(trailing-spaces)
[error] 126-126: trailing spaces
(trailing-spaces)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
[warning] 161-161: too many blank lines
(2 > 0) (empty-lines)
137-160: Verify Python version management strategy.The commented matrix job raises several concerns:
- How are different Python versions managed in the development image?
- What's the plan for transitioning from the container-based job to this matrix job?
- Consider adding input validation for Python version format.
Run this script to check Python version management in the development image:
🧰 Tools
🪛 yamllint (1.35.1)
[error] 141-141: trailing spaces
(trailing-spaces)
[error] 142-142: trailing spaces
(trailing-spaces)
alex-liang3
force-pushed
the
users/alexliang/test-matrix
branch
from
ab2d7e2 to
b63f7a3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
.github/workflows/test.yml (1)
177-200: 🛠️ Refactor suggestionRemove commented out job.
Similar to the previous comment, remove the commented-out job instead of keeping it in version control.
🧰 Tools
🪛 yamllint (1.35.1)
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 182-182: trailing spaces
(trailing-spaces)
🧹 Nitpick comments (4)
.github/workflows/test.yml (4)
16-19: LGTM! Consider enhancing the input parameter documentation.The optional input parameter is well-defined. However, the example could be more descriptive.
- test_python_versions: # e.g. "['3.11', '3.12', '3.13']" + test_python_versions: + # Optional: Specify Python versions as a JSON array string + # e.g. "['3.11', '3.12', '3.13']" + # If not provided, uses the default Python version from the Dockerfile
117-117: Fix trailing spaces.Remove trailing spaces from the indicated lines to maintain consistent formatting.
Also applies to: 140-140, 145-145, 146-146, 148-148, 175-175, 181-181, 182-182
🧰 Tools
🪛 yamllint (1.35.1)
[error] 117-117: trailing spaces
(trailing-spaces)
141-144: Improve Python version detection robustness.The current shell command for detecting Python versions could be fragile. Consider using Python's
packagingmodule for more reliable wheel filename parsing.- python_versions=$(echo -n "["; find packages/python -name "*.whl" | grep -o "py[0-9]\{2,3\}" | sed 's/py\([0-9]\)\([0-9][0-9]\?\)/\1.\2/' | sed "s/\([0-9.]*\)/'\1'/" | tr '\n' ',' | sed 's/,$//'; echo "]") + python_versions=$(python3 -c ' + from pathlib import Path + from packaging.tags import parse_wheel_filename + wheels = Path("packages/python").glob("*.whl") + versions = set() + for wheel in wheels: + _, _, _, tags = parse_wheel_filename(wheel.name) + for tag in tags: + if tag.interpreter: + versions.add(tag.interpreter.replace("py", "").replace("", ".")) + print(f"[\"{"\", \"".join(sorted(versions))}\"]") + ')🧰 Tools
🪛 actionlint (1.7.4)
141-141: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
201-201: Remove extra blank lines at the end of the file.Maintain a single newline at the end of the file.
🧰 Tools
🪛 yamllint (1.35.1)
[warning] 201-201: too many blank lines
(2 > 0) (empty-lines)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/build-packages.yml(2 hunks).github/workflows/release.yml(2 hunks).github/workflows/test.yml(3 hunks)
🧰 Additional context used
🪛 yamllint (1.35.1)
.github/workflows/test.yml
[error] 117-117: trailing spaces
(trailing-spaces)
[error] 140-140: trailing spaces
(trailing-spaces)
[error] 145-145: trailing spaces
(trailing-spaces)
[error] 146-146: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
[error] 175-175: trailing spaces
(trailing-spaces)
[error] 181-181: trailing spaces
(trailing-spaces)
[error] 182-182: trailing spaces
(trailing-spaces)
[warning] 201-201: too many blank lines
(2 > 0) (empty-lines)
🪛 actionlint (1.7.4)
.github/workflows/test.yml
141-141: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (3)
.github/workflows/test.yml (1)
146-174: LGTM! Well-structured matrix testing implementation.The job correctly:
- Uses matrix strategy with dynamically detected versions
- Downloads artifacts from the build job
- Runs tests for each Python version
🧰 Tools
🪛 yamllint (1.35.1)
[error] 146-146: trailing spaces
(trailing-spaces)
[error] 148-148: trailing spaces
(trailing-spaces)
.github/workflows/build-packages.yml (1)
62-62: LGTM! Good architecture-specific artifact naming.The artifact names now include the architecture, which is a good practice for clarity and supports the new multi-version testing strategy.
Also applies to: 100-100
.github/workflows/release.yml (1)
46-47: LGTM! Proper handling of multiple architecture artifacts.The changes correctly:
- Use patterns to match architecture-specific artifacts
- Enable merging of multiple artifacts
- Maintain consistent paths for downstream steps
Also applies to: 67-68
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
.github/workflows/test.yml (2)
116-120: Make Python version extraction more robust.The current version extraction logic might be fragile. Consider using Python's wheel package to parse wheel filenames more reliably.
Here's an alternative implementation:
import glob from wheel.wheelfile import parse_wheel_filename wheels = glob.glob('packages/python/*.whl') versions = set() for wheel in wheels: name, ver, build, tags = parse_wheel_filename(wheel.split('/')[-1]) for tag in tags: if tag.startswith('py'): versions.add(f"'{tag[2:3]}.{tag[3:]}'") print(f"value=[{','.join(sorted(versions))}]")This would require adding
wheelto the development container's Python packages.🧰 Tools
🪛 actionlint (1.7.4)
117-117: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
148-149: Add error handling for test command.The test command should handle failures gracefully and provide clear error messages.
Apply this diff:
- - name: Run Python Unit Tests - run: make ci-test-python ${{ format('test_python_version={0}', matrix.python-version) }} + - name: Run Python Unit Tests + run: | + echo "Running tests with Python ${{ matrix.python-version }}" + if ! make ci-test-python ${{ format('test_python_version={0}', matrix.python-version) }}; then + echo "::error::Tests failed for Python ${{ matrix.python-version }}" + exit 1 + fi
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
117-117: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
🔇 Additional comments (1)
.github/workflows/test.yml (1)
113-114: Clarify or remove the TBM comment.The comment suggests this is a temporary naming convention. Please either:
- Remove the comment if the naming is final
- Create an issue to track the planned naming changes
- Implement the final naming now
alex-liang3
force-pushed
the
fix/ci-v4-artifacts
branch
2 times, most recently
from
6e9ce02 to
5f0ab8f
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
.github/workflows/test.yml (3)
113-114: Specify artifact retention period and finalize namespace.The comment "TBM: use a different namespace than in build-packages for now" suggests this is temporary. Consider:
- Adding
retention-daysto manage artifact storage- Finalizing the artifact namespace naming strategy
with: name: python-bindings # TBM: use a different namespace than in build-packages for now. path: packages/python/ + retention-days: 1
149-149: Add error handling for Python version testing.The make command should validate the Python version format and handle errors appropriately.
-run: make ci-test-python ${{ format('test_python_version={0}', matrix.python-version) }} +run: | + if [[ ! "${{ matrix.python-version }}" =~ ^[0-9]+\.[0-9]+$ ]]; then + echo "Error: Invalid Python version format: ${{ matrix.python-version }}" + exit 1 + fi + make ci-test-python ${{ format('test_python_version={0}', matrix.python-version) }}
Line range hint
90-149: Implementation successfully achieves multi-version testing.The approach of separating build and test phases with artifact sharing is architecturally sound. The matrix strategy effectively enables parallel testing across Python versions. Consider these architectural points:
- The separation of concerns between build and test jobs improves maintainability
- Artifact sharing reduces redundant builds
- Matrix strategy enables efficient parallel testing
Future considerations:
- Consider caching Python packages to speed up builds
- Monitor artifact storage usage as the number of Python versions grows
- Add metrics collection for test execution times across versions
🧰 Tools
🪛 actionlint (1.7.4)
117-117: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
.github/workflows/test.yml(2 hunks)
🧰 Additional context used
🪛 actionlint (1.7.4)
.github/workflows/test.yml
117-117: shellcheck reported issue in this script: SC2086:info:3:36: Double quote to prevent globbing and word splitting
(shellcheck)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Build and Push and Tag Base Image with Version / Build Development Image
🔇 Additional comments (1)
.github/workflows/test.yml (1)
127-129: 🛠️ Refactor suggestionAdd matrix strategy safeguards.
The matrix strategy needs additional safeguards:
- Add fail-fast to stop all jobs when one fails
- Validate that Python versions list is not empty
- Add proper error messaging
strategy: + fail-fast: true matrix: python-version: ${{ fromJson(needs.build-python.outputs.python_versions) }} + if: ${{ needs.build-python.outputs.python_versions != '[]' }} + continue-on-error: falseLikely invalid or redundant comment.
!!! Assumes that the downstream repo has a new Makefile directive called
ci-test-python. Merge the following downstream PRs BEFORE this one:Adds the ability to unit test multiple python versions in CI. This will automatically detect and run tests for every version of Python that the downstream OSTk lib has been built for. A demo pipeline can be seen here: https://github.com/open-space-collective/open-space-toolkit-core/actions/runs/12712001256
Currently, this introduces a duplicate call to
make build-packages-python-standalonein order to decouple the Python building from the installation and testing (it's called again when the module is packaged) - this will likely be made more efficient in the future.Summary by CodeRabbit