Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. ✨ Finishing Touches🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Pull Request Overview
Updates GitHub Actions and dependency versions to keep templates current with the latest releases. Addresses issue #63 regarding outdated action versions.
- Updates GitHub Actions to newer versions across all workflow files
- Updates dependency versions for Python packages, Node.js packages, and Docker images
- Enhances SSM plugin installation with Windows support and improved argument handling
Reviewed Changes
Copilot reviewed 22 out of 23 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
| template/.pre-commit-config.yaml | Updates pre-commit hook versions for typos, hadolint, and ruff |
| template/.github/workflows/publish.yaml.jinja | Improves package import verification with enhanced checks and longer wait times |
| template/.github/workflows/pre-commit.yaml | Updates checkout and cache action versions, adds SSM plugin skip option |
| template/.github/workflows/get-values.yaml | Updates checkout action version |
| template/.github/actions/update-devcontainer-hash/action.yml | Updates checkout action version |
| template/.github/actions/install_deps/action.yml | Updates setup-python, setup-node, and AWS credentials actions, adds SSM plugin skip parameter |
| template/.devcontainer/install-ci-tooling.py.jinja | Adds Windows support for SSM plugin installation and improves argument handling |
| template/.devcontainer/devcontainer.json.jinja | Updates VS Code extension versions |
| pyproject.toml | Updates Python package dependencies to latest versions |
| extensions/context.py | Updates version constants for dependencies and GitHub Actions |
| .pre-commit-config.yaml | Updates pre-commit hook versions (same as template) |
| .github/workflows/tag-on-merge.yaml | Updates checkout action version |
| .github/workflows/pre-commit.yaml | Updates checkout and cache action versions, adds SSM plugin skip option |
| .github/workflows/get-values.yaml | Updates checkout action version |
| .github/workflows/ci.yaml | Updates checkout and cache action versions |
| .github/reusable_workflows/build-docker-image.yaml | Updates Docker setup and AWS actions, adds artifact output |
| .github/actions/update-devcontainer-hash/action.yml | Updates checkout action version |
| .github/actions/install_deps/action.yml | Updates setup actions and AWS credentials, adds SSM plugin skip parameter |
| .devcontainer/manual-setup-deps.sh | Removes deprecated file |
| .devcontainer/install-ci-tooling.py | Adds Windows SSM plugin support and improves argument handling |
| .devcontainer/devcontainer.json | Updates VS Code extension versions |
| .copier-answers.yml | Updates template commit reference |
Comments suppressed due to low confidence (1)
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.devcontainer/install-ci-tooling.py (1)
105-120: Avoid shell=True with list args; fix Windows/non-Windows invocationCurrently passing a list with shell=True; use shell=False for list invocations on Windows and a string with shell=True on POSIX to avoid edge cases.
- pnpm_install_sequence = ["npm -v", f"npm install -g pnpm@{PNPM_VERSION}", "pnpm -v"] - for cmd in pnpm_install_sequence: - cmd = ( - [ - pwsh, # type: ignore[reportPossiblyUnboundVariable] # this matches the conditional above that defines pwsh - "-NoProfile", - "-NonInteractive", - "-Command", - cmd, - ] - if is_windows - else [cmd] - ) - _ = subprocess.run(cmd, shell=True, check=True) + pnpm_install_sequence = ["npm -v", f"npm install -g pnpm@{PNPM_VERSION}", "pnpm -v"] + for cmd in pnpm_install_sequence: + if is_windows: + _ = subprocess.run( + [ + pwsh, # type: ignore[reportPossiblyUnboundVariable] + "-NoProfile", + "-NonInteractive", + "-Command", + cmd, + ], + check=True, + ) + else: + _ = subprocess.run(cmd, shell=True, check=True)
♻️ Duplicate comments (2)
.github/actions/update-devcontainer-hash/action.yml (1)
29-31: Same checkout v5 pinning recommendation applies here.Replicate commit-SHA pinning for consistency.
.github/workflows/pre-commit.yaml (1)
35-42: Checkout v5 upgrade is fine; apply SHA pin if you adopt that policy.Matches other workflows.
🧹 Nitpick comments (15)
.github/workflows/get-values.yaml (1)
27-28: Pin checkout to a specific SHA
Use:- name: Checkout code uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8to prevent tag drift. v5.0.0 has no functional breaking changes from v4.x (only moves to Node.js 24 and requires GitHub Actions Runner ≥ v2.327.1), so ensure self-hosted runners are updated if needed.
template/.github/actions/update-devcontainer-hash/action.yml (1)
29-31: Mirror the pinning strategy in template, too.Keep template and root consistent to avoid drift.
template/.github/workflows/publish.yaml.jinja (4)
189-189: Increase PyPI wait to 120s: good; add retry for robustnessConsider a small retry around pip install to smooth sporadic index propagation delays.
Example inline:
- name: Install from staging registry - run: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }} + run: | + for i in {1..5}; do + pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }} && break + echo "Install attempt $i failed; retrying in 15s..." + sleep 15 + done
195-205: Avoid accidental local imports during verificationEven with PYTHONPATH cleared, CWD can still be on sys.path. Run the check from a temp dir to ensure import is from site-packages.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources + working-directory: ${{ runner.temp }} run: | python - <<'PY' import sys, importlib, pathlib print(f"Python version {sys.version}") m = importlib.import_module("{{ package_name | replace('-', '_') }}") p = pathlib.Path(getattr(m, "__file__", "")) print(f"Imported from: {p}") assert "site-packages" in str(p), f"Expected site-packages, got {p}" PY
294-294: Primary install wait to 120s: good; mirror retry suggestionMirror the pip-install retry here as well to reduce flakes.
300-310: Apply the same clean-CWD import check on primaryReplicate the working-directory hardening here to avoid local import bleed-through.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources + working-directory: ${{ runner.temp }} run: | python - <<'PY' import sys, importlib, pathlib print(f"Python version {sys.version}") m = importlib.import_module("{{ package_name | replace('-', '_') }}") p = pathlib.Path(getattr(m, "__file__", "")) print(f"Imported from: {p}") assert "site-packages" in str(p), f"Expected site-packages, got {p}" PY.github/reusable_workflows/build-docker-image.yaml (1)
34-37: New reusable output (artifact-name): nice additionThis makes downstream composition cleaner. Consider documenting this in the workflow README.
.devcontainer/install-ci-tooling.py (1)
126-156: Consider checksum verification for downloaded installersOptional hardening: verify SHA256 for the downloaded EXE/DEB before executing/installing to mitigate supply-chain risk.
template/.devcontainer/install-ci-tooling.py.jinja (1)
156-156: Minor: Consider improving the output formatting.The print statement adds a colon but the actual version output will be on a new line, which might look slightly awkward.
Consider using
session-manager-plugin --versionoutputs on its own line:- print("SSM Plugin Manager Version: ") + print("SSM Plugin Manager Version:")Or combine them into a single output:
- print("SSM Plugin Manager Version: ") - _ = subprocess.run( - ["session-manager-plugin", "--version"], - check=True, - ) + result = subprocess.run( + ["session-manager-plugin", "--version"], + check=True, + capture_output=True, + text=True, + ) + print(f"SSM Plugin Manager Version: {result.stdout.strip()}")template/.github/workflows/get-values.yaml (1)
28-28: Consider pinning actions/checkout@v5.0.0 to a commit SHA
Upgrade to v5.0.0 looks good; for supply-chain hardening, pin the action to a commit SHA (and optionally enable Dependabot to bump SHAs) and confirm that v5 defaults (persist-credentials, fetch-depth) don’t affect your push step.template/.github/workflows/pre-commit.yaml (3)
35-35: Checkout@v5 on push path: OK; consider commit pinning.Looks consistent with your other workflows. Recommend pinning to a commit SHA for defense-in-depth.
41-41: Same here for non-push path.Functionally fine; prefer commit SHA pin to reduce supply-chain risk.
52-55: Mutex addition is reasonable; minor hygiene suggestion.Good use of a pinned commit. Consider prefixing the branch (e.g., gh-mutex/…) to keep refs tidy if many language/version combos accumulate.
template/.github/actions/install_deps/action.yml (2)
19-23: New inputskip-installing-ssm-plugin-manager: LGTM; document in consumer workflows.Input/description are clear and the behavior is gated. Ensure any workflow calling this action sets the flag explicitly if they rely on skipping SSM.
68-68: Flag passthrough to installer: LGTM; tiny robustness tweak possible.Current ternaries work; to avoid accidental double spaces or trailing blanks in pwsh logs, consider building the arg list conditionally in the script itself. Not required.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
uv.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.copier-answers.yml(1 hunks).devcontainer/devcontainer.json(2 hunks).devcontainer/install-ci-tooling.py(3 hunks).devcontainer/manual-setup-deps.sh(0 hunks).github/actions/install_deps/action.yml(2 hunks).github/actions/update-devcontainer-hash/action.yml(1 hunks).github/reusable_workflows/build-docker-image.yaml(4 hunks).github/workflows/ci.yaml(2 hunks).github/workflows/get-values.yaml(1 hunks).github/workflows/pre-commit.yaml(2 hunks).github/workflows/tag-on-merge.yaml(1 hunks).pre-commit-config.yaml(2 hunks)extensions/context.py(2 hunks)pyproject.toml(1 hunks)template/.devcontainer/devcontainer.json.jinja(1 hunks)template/.devcontainer/install-ci-tooling.py.jinja(2 hunks)template/.github/actions/install_deps/action.yml(2 hunks)template/.github/actions/update-devcontainer-hash/action.yml(1 hunks)template/.github/workflows/get-values.yaml(1 hunks)template/.github/workflows/pre-commit.yaml(2 hunks)template/.github/workflows/publish.yaml.jinja(2 hunks)template/.pre-commit-config.yaml(2 hunks)
💤 Files with no reviewable changes (1)
- .devcontainer/manual-setup-deps.sh
🧰 Additional context used
🪛 Ruff (0.12.2)
.devcontainer/install-ci-tooling.py
126-126: subprocess call: check for execution of untrusted input
(S603)
127-132: Starting a process with a partial executable path
(S607)
135-135: subprocess call: check for execution of untrusted input
(S603)
143-143: subprocess call: check for execution of untrusted input
(S603)
144-149: Starting a process with a partial executable path
(S607)
152-152: subprocess call: check for execution of untrusted input
(S603)
153-153: Starting a process with a partial executable path
(S607)
🔇 Additional comments (36)
.copier-answers.yml (1)
2-2: Template bump acknowledged._commit advanced to v0.0.69; no issues spotted.
.github/workflows/tag-on-merge.yaml (1)
16-16: Checkout v5 bump looks good.No logic changes; proceed.
pyproject.toml (1)
9-15: InstalluvCLI in CI and validate lock alignment. Ensure theuvtool is available before runninguv lock --checkto update and verify the lock file against your bumped deps, and confirm CI runners install Node sopyright[nodejs]>=1.1.405can execute successfully..github/workflows/pre-commit.yaml (2)
58-58: Cache action bump LGTM.Minor patch update; safe.
48-48: skip-installing-ssm-plugin-manager is already defined and wired in install_deps action
The input appears in both.github/actions/install_deps/action.ymland the template, and is passed through to the install-ci-tooling script..pre-commit-config.yaml (3)
45-45: Typos hook rev bump LGTM.
221-221: Hadolint rev bump LGTM.
229-229: Ruff hook rev bump LGTM..github/workflows/ci.yaml (2)
56-56: Checkout v5 bump: looks good; confirm runner Node 20 availabilityactions/checkout@v5 requires Node 20 on the runner. GitHub-hosted runners already provide this, but please confirm no self-hosted runners are affected.
111-111: Cache action patch bump: LGTMMinor patch from v4.2.3 to v4.2.4—safe.
template/.pre-commit-config.yaml (3)
45-45: Typos hook rev bump: LGTMPinned to a newer frozen SHA; no config changes needed.
221-221: Hadolint hook rev bump: LGTMGood to keep the Dockerfile linter current.
229-229: Ruff pre-commit rev bump: verify toolchain alignmentEnsure the ruff version used by CI matches this hook rev to avoid unexpected rule changes.
.github/reusable_workflows/build-docker-image.yaml (4)
47-49: Job outputs wired correctlyOutput references the hash-derived image name; matches the new workflow_call output.
68-68: Checkout v5 bump: LGTM; confirm Node 20As elsewhere, confirm no self-hosted runners with older Node.
72-72: aws-actions/configure-aws-credentials v5: LGTMOIDC auth remains the same; good to be on latest.
123-126: Buildx action/version pin: good practicePinning setup-buildx-action and BuildKit version helps reproducibility.
template/.devcontainer/devcontainer.json.jinja (1)
32-47: Extension version bumps: LGTMKeeping Copilot, Python, Volar, and ESLint current is fine; pinned versions aid determinism.
.devcontainer/devcontainer.json (1)
26-32: Extension version bumps: LGTMMatches template; deterministic builds retained.
.devcontainer/install-ci-tooling.py (1)
10-13: Tool versions bumped: LGTMUV, PNPM, Copier, and template extensions updated appropriately.
template/.devcontainer/install-ci-tooling.py.jinja (4)
34-38: LGTM! The flag rename improves clarity.The change from
--install-ssm-pluginto--skip-installing-ssm-pluginfollows the common convention where flags explicitly state what they skip/disable, making the intent clearer.
120-120: Good implementation of the conditional logic.The condition correctly combines the template-based default setting with the user's runtime preference, ensuring the SSM plugin is only installed when both conditions are met.
122-138: Windows SSM plugin installation looks correct.The Windows-specific implementation properly:
- Downloads the correct installer from AWS
- Uses the
/quietflag for unattended installation- Pins the version for consistency
139-155: Linux/macOS implementation maintains consistency.The implementation correctly uses
dpkgfor Debian-based systems and maintains version parity with the Windows installer (1.2.707.0)..github/actions/install_deps/action.yml (3)
19-23: LGTM! Well-documented input parameter.The new
skip-installing-ssm-plugin-managerinput is properly defined with appropriate defaults and clear description.
56-56: Action version updates look good.The GitHub Action version updates are appropriate:
actions/setup-python: v5.6.0 → v6.0.0actions/setup-node: v4.4.0 → v5.0.0aws-actions/configure-aws-credentials: v4.2.0 → v5.0.0These are all major version bumps that should be tested in downstream repositories.
Also applies to: 62-62, 73-73
68-68: Correct implementation of the conditional flag.The ternary operator correctly passes
--skip-installing-ssm-pluginwhen the input is true, maintaining consistency with the Python script's expectations.extensions/context.py (4)
13-14: Tool version updates are reasonable.The UV and pnpm version updates are minor/patch releases:
- UV: 0.8.4 → 0.8.15 (patch releases)
- pnpm: 10.14.0 → 10.15.1 (minor + patch)
These should be backward compatible.
36-36: New JavaScript/TypeScript dependencies added.Several new dependencies have been added to support frontend development:
fastapi_offline_version: 1.7.4dotenv_cli_version: ^9.0.0vue_tsc_version: ^3.0.6vue_devtools_api_version: ^8.0.0vitest_version: ^3.2.4nuxt_eslint_version: ^1.9.0zod_from_json_schema_version: ^0.5.0types_node_version: ^24.3.1These additions suggest expanded frontend/TypeScript support in the template.
Also applies to: 45-45, 51-58
63-77: GitHub Actions version updates are comprehensive.All GitHub Actions have been updated to their latest stable versions. The notable updates include:
- Major version bumps for checkout (v4→v5), setup-python (v5→v6), download-artifact (v4→v5), setup-node (v4→v5), configure-aws-credentials (v4→v5)
- Minor/patch updates for other actions
These updates align with GitHub's recommendations for using the latest action versions.
87-88: Container base image updates.Alpine and nginx base images have been updated:
- Alpine: 3.21 → 3.22
- nginx: 1.28.0 → 1.29.1
Both are sensible updates to newer stable versions.
template/.github/workflows/pre-commit.yaml (2)
48-48: New input wiring for SSM skip flag: LGTM.Input name matches the composite action’s new input and maps to the expected CLI flag downstream.
58-58: Cache bump to v4.2.4: LGTM.No concerns.
template/.github/actions/install_deps/action.yml (3)
56-56: actions/setup-python@v6.0.0: verify compatibility in all callers.The bump is fine here. Double-check other workflows using this composite action don’t rely on deprecated v5 behaviors.
62-62: actions/setup-node@v5.0.0: OK.No issues anticipated.
73-73: aws-actions/configure-aws-credentials@v5.0.0: ensure permissions present in callers.This composite assumes id-token: write at the workflow level. Confirm all workflows that set a non-default role name also grant id-token: write and provide a region.
| "--skip-installing-ssm-plugin", | ||
| action="store_true", | ||
| default=INSTALL_SSM_PLUGIN_BY_DEFAULT, | ||
| help="Install the SSM plugin for AWS CLI", | ||
| default=False, | ||
| help="Skip installing the SSM plugin for AWS CLI", | ||
| ) |
There was a problem hiding this comment.
SSM plugin install is effectively disabled by default
With INSTALL_SSM_PLUGIN_BY_DEFAULT=False and a “skip” flag, the install block never runs unless the constant is flipped. Given the rename to a skip flag, default should likely be to install unless skipped.
- INSTALL_SSM_PLUGIN_BY_DEFAULT = False
+ INSTALL_SSM_PLUGIN_BY_DEFAULT = TrueAlso, the immediate version check may fail if PATH isn’t refreshed (especially on Windows). Resolve the binary via shutil.which with sane fallbacks:
- print("SSM Plugin Manager Version: ")
- _ = subprocess.run(
- ["session-manager-plugin", "--version"],
- check=True,
- )
+ print("SSM Plugin Manager Version: ")
+ smp = shutil.which("session-manager-plugin")
+ if not smp and is_windows:
+ # default install path on GitHub Windows runners
+ candidate = r"C:\Program Files\Amazon\SessionManagerPlugin\bin\session-manager-plugin.exe"
+ smp = candidate if Path(candidate).exists() else None
+ if not smp and not is_windows:
+ # common linux paths
+ for candidate in ("/usr/local/bin/session-manager-plugin", "/usr/bin/session-manager-plugin",
+ "/usr/local/sessionmanagerplugin/bin/session-manager-plugin"):
+ if Path(candidate).exists():
+ smp = candidate
+ break
+ if not smp:
+ raise FileNotFoundError("session-manager-plugin not found on PATH after install")
+ _ = subprocess.run([smp, "--version"], check=True)Also applies to: 120-160
| context["pulumi_version"] = "3.193.0" | ||
| context["pulumi_aws_version"] = "7.7.0" | ||
| context["pulumi_aws_native_version"] = "1.33.0" | ||
| context["pulumi_command_version"] = "1.1.0" | ||
| context["pulumi_github_version"] = "6.7.3" | ||
| context["pulumi_okta_version"] = "4.20.0" | ||
| context["boto3_version"] = "1.40.9" | ||
| context["ephemeral_pulumi_deploy_version"] = "0.0.4" | ||
| context["pulumi_okta_version"] = "5.2.0" | ||
| context["boto3_version"] = "1.40.25" | ||
| context["ephemeral_pulumi_deploy_version"] = "0.0.5" |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Pulumi and AWS SDK updates look good.
The Pulumi ecosystem updates include:
- Core Pulumi: 3.190.0 → 3.193.0
- AWS provider: 7.4.0 → 7.7.0
- Okta provider: 4.20.0 → 5.2.0 (major version bump)
- boto3: 1.40.9 → 1.40.25
Note that the Okta provider has a major version bump which may introduce breaking changes.
🌐 Web query:
Pulumi Okta provider breaking changes version 5.0.0
💡 Result:
Breaking changes in pulumi-okta v5.0.0
- The resource okta:auth/serverPolicyClaim:ServerPolicyClaim was removed (missing in v5.0.0). [1]
- v5.0.0 upgrades the underlying terraform-provider-okta to a v5.x major release (see release notes). [1]
Additional context: the Pulumi Okta registry notes also show v5.x provider pages and migration guidance (e.g., provider source/configuration changes). [2]
Sources:
[1] pulumi/pulumi-okta GitHub releases — v5.0.0.
[2] Pulumi Registry — Okta provider package (v5.0.0).
Approve Pulumi and AWS SDK updates; address Okta provider v5 breaking changes
- Pulumi core (3.190.0 → 3.193.0), AWS provider (7.4.0 → 7.7.0) and boto3 (1.40.9 → 1.40.25) updates approved.
- Okta provider bumped from 4.20.0 → 5.2.0 includes breaking changes in v5.0.0: the
okta:auth/serverPolicyClaimresource was removed and the underlying Terraform provider upgraded to v5.x; verify there are no references toserverPolicyClaimin your code and follow the Pulumi Registry migration guide for any required configuration/source updates.
🤖 Prompt for AI Agents
In extensions/context.py around lines 23 to 30, the Okta provider was updated to
v5.2.0 which includes breaking changes (removal of okta:auth/serverPolicyClaim
and Terraform provider upgrade); search the repo for any references to
serverPolicyClaim or related v4 APIs and replace them with the v5 equivalents
per the Pulumi Registry migration guide, update any resource or data source
names/arguments to match the new provider schema, run Pulumi preview and
reconcile or migrate state as recommended by the guide, and then commit the code
and updated tests/infra smoke checks verifying no remaining references to the
removed resource.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
template/.github/workflows/publish.yaml.jinja (4)
186-189: Replace fixed sleeps with registry-availability polling (reduces flakes, speeds happy path).A hard 120s sleep is brittle and slows runs when the package appears earlier. Poll the PyPI/TestPyPI JSON endpoint until the specific version is available, with a bounded timeout.
Apply these diffs:
- - name: Sleep to allow PyPI Index to update before proceeding to the next step - uses: juliangruber/sleep-action@{{ gha_sleep }} - with: - time: 120s + - name: Wait for Test PyPI to serve the new version + run: | + set -euo pipefail + PKG="{{ package_name | replace('_', '-') }}" + VER="${{ needs.get-values.outputs.package-version }}" + for i in $(seq 1 60); do + code="$(curl -fsS -o /dev/null -w '%{http_code}' "https://test.pypi.org/pypi/${PKG}/${VER}/json" || true)" + if [ "$code" = "200" ]; then + echo "Found ${PKG}==${VER} on Test PyPI." + exit 0 + fi + echo "Not yet available; sleeping 5s..." + sleep 5 + done + echo "Timeout waiting for ${PKG}==${VER} on Test PyPI." + exit 1- - name: Sleep to allow PyPI Index to update before proceeding to the next step - uses: juliangruber/sleep-action@{{ gha_sleep }} - with: - time: 120s + - name: Wait for PyPI to serve the new version + run: | + set -euo pipefail + PKG="{{ package_name | replace('_', '-') }}" + VER="${{ needs.get-values.outputs.package-version }}" + for i in $(seq 1 60); do + code="$(curl -fsS -o /dev/null -w '%{http_code}' "https://pypi.org/pypi/${PKG}/${VER}/json" || true)" + if [ "$code" = "200" ]; then + echo "Found ${PKG}==${VER} on PyPI." + exit 0 + fi + echo "Not yet available; sleeping 5s..." + sleep 5 + done + echo "Timeout waiting for ${PKG}==${VER} on PyPI." + exit 1Also applies to: 291-294
195-205: Harden import verification: allow dist-packages and assert the installed version.Current check fails on Debian-style dist-packages and doesn’t verify the exact version. Improve robustness and signal mismatches early.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources run: | - python - <<'PY' - import sys, importlib, pathlib - print(f"Python version {sys.version}") - m = importlib.import_module("{{ package_name | replace('-', '_') }}") - p = pathlib.Path(getattr(m, "__file__", "")) - print(f"Imported from: {p}") - assert "site-packages" in str(p), f"Expected site-packages, got {p}" - PY + python - <<'PY' + import sys, importlib, importlib.util, importlib.metadata as md, pathlib + print(f"Python version {sys.version}") + mod_name = "{{ package_name | replace('-', '_') }}" + dist_name = "{{ package_name | replace('_', '-') }}" + m = importlib.import_module(mod_name) + spec = importlib.util.find_spec(mod_name) + origin = (getattr(spec, "origin", None) or getattr(m, "__file__", "")) or "" + p = pathlib.Path(origin) + print(f"Imported from: {p}") + assert any(s in str(p) for s in ("site-packages", "dist-packages")), f"Expected site/dist-packages, got {p}" + expected = "${{ needs.get-values.outputs.package-version }}" + installed = md.version(dist_name) + print(f"Installed distribution version: {installed} (expected {expected})") + assert installed == expected, f"Version mismatch: expected {expected}, got {installed}" + PY
300-310: Mirror the hardened import verification in the primary-install job.Keep the verification logic consistent across staging and primary.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources run: | - python - <<'PY' - import sys, importlib, pathlib - print(f"Python version {sys.version}") - m = importlib.import_module("{{ package_name | replace('-', '_') }}") - p = pathlib.Path(getattr(m, "__file__", "")) - print(f"Imported from: {p}") - assert "site-packages" in str(p), f"Expected site-packages, got {p}" - PY + python - <<'PY' + import sys, importlib, importlib.util, importlib.metadata as md, pathlib + print(f"Python version {sys.version}") + mod_name = "{{ package_name | replace('-', '_') }}" + dist_name = "{{ package_name | replace('_', '-') }}" + m = importlib.import_module(mod_name) + spec = importlib.util.find_spec(mod_name) + origin = (getattr(spec, "origin", None) or getattr(m, "__file__", "")) or "" + p = pathlib.Path(origin) + print(f"Imported from: {p}") + assert any(s in str(p) for s in ("site-packages", "dist-packages")), f"Expected site/dist-packages, got {p}" + expected = "${{ needs.get-values.outputs.package-version }}" + installed = md.version(dist_name) + print(f"Installed distribution version: {installed} (expected {expected})") + assert installed == expected, f"Version mismatch: expected {expected}, got {installed}" + PY
209-209: Use fromJSON for boolean inputs to avoid string-comparison pitfalls.String checks work, but fromJSON is the recommended pattern for typed workflow_dispatch booleans and avoids surprises.
- if: ${{ github.event.inputs.publish_to_primary == 'true' }} + if: ${{ fromJSON(github.event.inputs.publish_to_primary || 'false') }}Apply the same to the publish-to-primary and install-from-primary jobs.
Also applies to: 228-228, 263-263
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template/.github/workflows/publish.yaml.jinja(4 hunks)
🔇 Additional comments (2)
template/.github/workflows/publish.yaml.jinja (2)
189-189: Confirm 60s is sufficient for PyPI/TestPyPI index propagation.60s can be tight; intermittent “No matching distribution found” happens. Consider bumping to 90–120s or adding a small retry.
- time: 60s + time: 90sAlso applies to: 294-294
209-209: LGTM: explicit gating on publish_to_primary.Comparing the boolean input to 'true' avoids accidental publishes. Looks good.
Also applies to: 228-228, 263-263
| time: 60s{% endraw %}{% endif %}{% raw %} | ||
| - name: Install from staging registry | ||
| run: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {% endraw %}{{ package_name | replace('_', '-') }}{% raw %}==${{ needs.get-values.outputs.package-version }} | ||
| run: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {% endraw %}{{ package_name | replace('_', '-') }}{% raw %}==${{ needs.get-values.outputs.package-version }} --no-cache |
There was a problem hiding this comment.
Fix pip flag: use --no-cache-dir (not --no-cache).
pip doesn’t support --no-cache; this will fail the install steps.
- run: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }} --no-cache
+ run: pip install --no-cache-dir --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }}
- run: pip install {{ package_name | replace('_', '-') }}=={{ needs.get-values.outputs.package-version }} --no-cache
+ run: pip install --no-cache-dir {{ package_name | replace('_', '-') }}=={{ needs.get-values.outputs.package-version }}Also applies to: 296-296
🤖 Prompt for AI Agents
In template/.github/workflows/publish.yaml.jinja around lines 191 and 296, the
pip install command uses the invalid flag --no-cache; replace it with the
correct --no-cache-dir flag in both occurrences so pip install commands succeed
(update the template string(s) accordingly where package_name and
package-version are interpolated).
| env: | ||
| PYTHONPATH: "" # avoid picking up local sources | ||
| run: | | ||
| python - <<'PY' | ||
| import sys, importlib, pathlib | ||
| print(f"Python version {sys.version}") | ||
| m = importlib.import_module("{% endraw %}{{ package_name | replace('-', '_') }}{% raw %}") | ||
| p = pathlib.Path(getattr(m, "__file__", "")) | ||
| print(f"Imported from: {p}") | ||
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | ||
| PY |
There was a problem hiding this comment.
Make the heredoc step Windows-safe by forcing a bash shell.
Here-doc syntax breaks under the default PowerShell on Windows runners. Pin the shell to bash for this step.
- name: Confirm library can be imported successfully
env:
PYTHONPATH: "" # avoid picking up local sources
+ shell: bash
run: |
python - <<'PY'
import sys, importlib, pathlib
print(f"Python version {sys.version}")
m = importlib.import_module("{{ package_name | replace('-', '_') }}")
p = pathlib.Path(getattr(m, "__file__", ""))
print(f"Imported from: {p}")
assert "site-packages" in str(p), f"Expected site-packages, got {p}"
PY📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| PYTHONPATH: "" # avoid picking up local sources | |
| run: | | |
| python - <<'PY' | |
| import sys, importlib, pathlib | |
| print(f"Python version {sys.version}") | |
| m = importlib.import_module("{% endraw %}{{ package_name | replace('-', '_') }}{% raw %}") | |
| p = pathlib.Path(getattr(m, "__file__", "")) | |
| print(f"Imported from: {p}") | |
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | |
| PY | |
| - name: Confirm library can be imported successfully | |
| env: | |
| PYTHONPATH: "" # avoid picking up local sources | |
| shell: bash | |
| run: | | |
| python - <<'PY' | |
| import sys, importlib, pathlib | |
| print(f"Python version {sys.version}") | |
| m = importlib.import_module("{{ package_name | replace('-', '_') }}") | |
| p = pathlib.Path(getattr(m, "__file__", "")) | |
| print(f"Imported from: {p}") | |
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | |
| PY |
🤖 Prompt for AI Agents
In template/.github/workflows/publish.yaml.jinja around lines 195 to 205, the
here-doc Python invocation fails on Windows runners because the default shell is
PowerShell; make the step explicitly use bash by adding a shell: bash property
for that run step (or change the run to use bash -lc) so the <<'PY' heredoc is
interpreted correctly, keeping the quoted delimiter as-is.
| env: | ||
| PYTHONPATH: "" # avoid picking up local sources | ||
| run: | | ||
| python - <<'PY' | ||
| import sys, importlib, pathlib | ||
| print(f"Python version {sys.version}") | ||
| m = importlib.import_module("{% endraw %}{{ package_name | replace('-', '_') }}{% raw %}") | ||
| p = pathlib.Path(getattr(m, "__file__", "")) | ||
| print(f"Imported from: {p}") | ||
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | ||
| PY{% endraw %} |
There was a problem hiding this comment.
Do the same for the primary import verification step.
Mirror the bash shell pin to keep Windows runners happy.
- name: Confirm library can be imported successfully
env:
PYTHONPATH: "" # avoid picking up local sources
+ shell: bash
run: |
python - <<'PY'
import sys, importlib, pathlib
print(f"Python version {sys.version}")
m = importlib.import_module("{{ package_name | replace('-', '_') }}")
p = pathlib.Path(getattr(m, "__file__", ""))
print(f"Imported from: {p}")
assert "site-packages" in str(p), f"Expected site-packages, got {p}"
PY📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| env: | |
| PYTHONPATH: "" # avoid picking up local sources | |
| run: | | |
| python - <<'PY' | |
| import sys, importlib, pathlib | |
| print(f"Python version {sys.version}") | |
| m = importlib.import_module("{% endraw %}{{ package_name | replace('-', '_') }}{% raw %}") | |
| p = pathlib.Path(getattr(m, "__file__", "")) | |
| print(f"Imported from: {p}") | |
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | |
| PY{% endraw %} | |
| - name: Confirm library can be imported successfully | |
| env: | |
| PYTHONPATH: "" # avoid picking up local sources | |
| shell: bash | |
| run: | | |
| python - <<'PY' | |
| import sys, importlib, pathlib | |
| print(f"Python version {sys.version}") | |
| m = importlib.import_module("{{ package_name | replace('-', '_') }}") | |
| p = pathlib.Path(getattr(m, "__file__", "")) | |
| print(f"Imported from: {p}") | |
| assert "site-packages" in str(p), f"Expected site-packages, got {p}" | |
| PY |
🤖 Prompt for AI Agents
In template/.github/workflows/publish.yaml.jinja around lines 300 to 310, the
primary import verification step needs the same Windows-friendly pin as the
secondary check; update that step to set PYTHONPATH: "" in env and run the
import verification via an inline python heredoc (python - <<'PY') that imports
the package by name using importlib.import_module("{% endraw %}{{ package_name |
replace('-', '_') }}{% raw %}"), prints the Python version and import path, and
asserts the module was loaded from site-packages, mirroring the existing
bash-locked snippet so Windows runners behave the same.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (4)
template/.github/workflows/publish.yaml.jinja (4)
191-191: Fix pip flag: use --no-cache-dir (not --no-cache).pip doesn’t support --no-cache; installs will fail.
- - name: Install from staging registry - run: pip install --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }} --no-cache + - name: Install from staging registry + run: pip install --no-cache-dir --index-url https://test.pypi.org/simple/ --extra-index-url https://www.pypi.org/simple {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }}
195-205: Pin shell to bash for heredoc.PowerShell on Windows won’t parse <<'PY'; step will fail on Windows runners.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources + shell: bash run: | python - <<'PY' import sys, importlib, pathlib print(f"Python version {sys.version}") m = importlib.import_module("{{ package_name | replace('-', '_') }}") p = pathlib.Path(getattr(m, "__file__", "")) print(f"Imported from: {p}") assert "site-packages" in str(p), f"Expected site-packages, got {p}" PY
296-296: Fix pip flag: use --no-cache-dir (not --no-cache).Same issue as staging install; this will fail as written.
- - name: Install from primary registry - run: pip install {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }} --no-cache + - name: Install from primary registry + run: pip install --no-cache-dir {{ package_name | replace('_', '-') }}==${{ needs.get-values.outputs.package-version }}
300-310: Pin shell to bash for heredoc (primary import check).Mirror the bash pin here to avoid Windows failures.
- name: Confirm library can be imported successfully env: PYTHONPATH: "" # avoid picking up local sources + shell: bash run: | python - <<'PY' import sys, importlib, pathlib print(f"Python version {sys.version}") m = importlib.import_module("{{ package_name | replace('-', '_') }}") p = pathlib.Path(getattr(m, "__file__", "")) print(f"Imported from: {p}") assert "site-packages" in str(p), f"Expected site-packages, got {p}" PY
🧹 Nitpick comments (2)
template/.github/workflows/publish.yaml.jinja (2)
189-189: Consider longer wait or a retry for index propagation.60s can be flaky for Test PyPI. Suggest 90s (or wrap install in a retry loop).
- time: 60s + time: 90s
294-294: Same flakiness concern for PyPI index.Bump to 90s or implement a retry around pip install.
- time: 60s + time: 90s
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
template/.github/workflows/publish.yaml.jinja(4 hunks)
🔇 Additional comments (2)
template/.github/workflows/publish.yaml.jinja (2)
209-209: Good: boolean input gating via fromJSON.Correctly parses workflow_dispatch boolean input.
228-228: Good: consistent gating for publish and install-from-primary.fromJSON usage is correct and consistent across steps.
Also applies to: 263-263
Why is this change necessary?
Keep up to date
How does this change address the issue?
Pulls in upstream changes
What side effects does this change have?
None
How is this change tested?
downstream repo
Other
also addresses #63 and #67
Summary by CodeRabbit
New Features
Chores