Switch Kitmaker to use Artifactory#292
Conversation
📝 WalkthroughWalkthroughRemoved GitHub Release publishing; Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as CI Runner
participant PW as publish-wheel job
participant AF as Artifactory API
participant KM as kitmaker job
Runner->>PW: remove local wheels/ directory
PW->>AF: upload wheel files (twine)
AF-->>PW: upload confirmation / artifact IDs
PW->>AF: search artifacts -> request metadata (downloadUri)
AF-->>PW: return downloadUri(s)
PW->>PW: strip base URL/repo -> produce repo-relative wheel_paths
PW-->>Runner: set job output wheel_paths
Runner->>KM: start kitmaker (depends on publish-wheel)
KM->>KM: validate ARTIFACTORY_URL (HTTPS), ARTIFACTORY_REPO, WHEEL_PATHS
KM->>KM: construct full wheel URLs = ARTIFACTORY_URL + ARTIFACTORY_REPO + wheel_path
KM->>KM: build submission payload (mask secrets)
KM->>KM: submit payload / report status
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Comment |
There was a problem hiding this comment.
Pull request overview
Updates the Ubuntu CI workflow to stop using temporary GitHub Releases as an intermediate wheel distribution mechanism and instead have Kitmaker consume wheels directly from internal Artifactory.
Changes:
- Expose
publish-wheeljob output (wheel_paths) derived from Artifactory metadata after upload. - Remove the
publish-github-release-assetsandcleanup-github-release-assetsjobs and rewire Kitmaker to depend onpublish-wheel. - Add a cleanup step to ensure the wheels download directory is clean before fetching artifacts.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
You can also share your feedback on Copilot code review. Take the survey.
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 361-364: The consumer job is reconstructing artifact URLs from
ARTIFACTORY_URL/ARTIFACTORY_REPO which can drift between environments; change
the producer job publish-wheel to export the full verified downloadUri (e.g.,
outputs.downloadUris or outputs.downloadUri_base) instead of only the relative
path, and update the consumer to use needs.publish-wheel.outputs.downloadUris
(or the new full downloadUri output) in place of recomputing the URL (replace
usage of WHEEL_PATHS + ARTIFACTORY_URL/ARTIFACTORY_REPO); ensure publish-wheel
sets the GitHub Actions output and the consumer job reads that output and
retains KITMAKER_UPLOAD behavior.
- Around line 301-333: search_json parsing currently uses .results[0] which can
be non-deterministic; change the logic around search_json/storage_uri to verify
the search returned exactly one result (e.g. use jq to compute .results |
length) and fail with an explanatory error (including search_json) if length !=
1, then only extract storage_uri from the sole result; update the error paths
that reference wheel_name and metadata_json so the workflow exits instead of
proceeding to compute wheel_path and append to wheel_paths when multiple or zero
matches are found.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a17fc978-6c58-4fb8-a65d-81eca000e4ba
📒 Files selected for processing (1)
.github/workflows/build-ubuntu.yml
There was a problem hiding this comment.
♻️ Duplicate comments (2)
.github/workflows/build-ubuntu.yml (2)
361-364:⚠️ Potential issue | 🟠 MajorCross-environment secret drift risk persists.
The previous review flagged that
publish-wheel(running underdevenvironment) resolves the verifieddownloadUribut only passes the relative path. This job (running underreleaseenvironment) reconstructs the full URL using its ownARTIFACTORY_URLandARTIFACTORY_REPOsecrets. If these secrets differ between environments, kitmaker will receive incorrect URLs.Consider exporting the full verified
downloadUri(or at least the base URL) frompublish-wheelinstead of only the relative path to eliminate cross-environment configuration drift.,
Also applies to: 388-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 361 - 364, The workflow currently uses needs.publish-wheel.outputs.wheel_paths (WHEEL_PATHS) and reconstructs full artifact URLs using ARTIFACTORY_URL/ARTIFACTORY_REPO in the release job (and gates behavior with KITMAKER_UPLOAD), causing cross-environment secret drift if those secrets differ; change the publish-wheel job to export the fully-qualified downloadUri(s) (or at minimum the base URL) as an output (e.g., publish-wheel.outputs.wheel_uris) and update this job to consume that output instead of rebuilding URLs from ARTIFACTORY_URL/ARTIFACTORY_REPO so kitmaker receives the verified full URLs regardless of environment.
301-312:⚠️ Potential issue | 🟠 MajorRequire exactly one Artifactory match before using
.results[0].The previous review flagged that
/api/search/artifactcan return multiple hits when files with identical names exist at different paths, and results are unordered. The code still selects.results[0].uriwithout verifying uniqueness, making the resolved wheel path non-deterministic.🛡️ Suggested guard
- storage_uri="$(jq -r '.results[0].uri // empty' <<< "${search_json}")" + result_count="$(jq '.results | length' <<< "${search_json}")" + if (( result_count != 1 )); then + echo "Expected exactly one Artifactory match for ${wheel_name}, got ${result_count}" + echo "${search_json}" + exit 1 + fi + storage_uri="$(jq -r '.results[0].uri' <<< "${search_json}")",
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 301 - 312, The current selection of storage_uri uses jq '.results[0].uri' on search_json, but the Artifactory /api/search/artifact can return multiple unordered hits; update the logic around search_json/storage_uri to require exactly one match for wheel_name: use jq to check '.results | length' (or otherwise count results) and if the count is not equal to 1, emit the full search_json and exit nonzero; only when the count is exactly 1 should you extract the single '.results[0].uri' into storage_uri and proceed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 361-364: The workflow currently uses
needs.publish-wheel.outputs.wheel_paths (WHEEL_PATHS) and reconstructs full
artifact URLs using ARTIFACTORY_URL/ARTIFACTORY_REPO in the release job (and
gates behavior with KITMAKER_UPLOAD), causing cross-environment secret drift if
those secrets differ; change the publish-wheel job to export the fully-qualified
downloadUri(s) (or at minimum the base URL) as an output (e.g.,
publish-wheel.outputs.wheel_uris) and update this job to consume that output
instead of rebuilding URLs from ARTIFACTORY_URL/ARTIFACTORY_REPO so kitmaker
receives the verified full URLs regardless of environment.
- Around line 301-312: The current selection of storage_uri uses jq
'.results[0].uri' on search_json, but the Artifactory /api/search/artifact can
return multiple unordered hits; update the logic around search_json/storage_uri
to require exactly one match for wheel_name: use jq to check '.results | length'
(or otherwise count results) and if the count is not equal to 1, emit the full
search_json and exit nonzero; only when the count is exactly 1 should you
extract the single '.results[0].uri' into storage_uri and proceed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 30efc95c-2755-4499-b267-037bf3fb61a3
📒 Files selected for processing (1)
.github/workflows/build-ubuntu.yml
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
.github/workflows/build-ubuntu.yml (2)
361-364:⚠️ Potential issue | 🟠 MajorPass the verified download URL across jobs instead of rebuilding it.
publish-wheel(running underdevenvironment) resolves the exactdownloadUrifrom Artifactory but outputs only the relative path. Thiskitmakerjob (running underreleaseenvironment) then reconstructs the full URL using its ownARTIFACTORY_URLandARTIFACTORY_REPOsecrets. If these secrets differ between environments, kitmaker will submit invalid URLs to Kitmaker.Consider exporting the full
downloadUri(or verified base URL) frompublish-wheelinstead of only the relative path.Also applies to: 388-388, 394-394
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 361 - 364, The kitmaker job rebuilds artifact URLs from ARTIFACTORY_URL and ARTIFACTORY_REPO instead of using the verified downloadUri produced by publish-wheel, which breaks when secrets differ between environments; modify the publish-wheel job to output the full downloadUri (or a verified base URL) as a workflow output (e.g., add an output like full_download_uris) and change the kitmaker job to consume that output (instead of reconstructing with ARTIFACTORY_URL/ARTIFACTORY_REPO) by reading needs.publish-wheel.outputs.full_download_uris (or the chosen name) and using that value when setting WHEEL_PATHS/KITMAKER_UPLOAD payloads so the exact validated URL from publish-wheel is passed across jobs.
301-312:⚠️ Potential issue | 🟠 MajorRequire a unique Artifactory match before using
.results[0].The Artifactory
/api/search/artifactAPI can return multiple results when files with identical names exist at different paths. Line 307 blindly selects the first result without verifying uniqueness, making the resolved wheel path non-deterministic.🔎 Suggested guard
- storage_uri="$(jq -r '.results[0].uri // empty' <<< "${search_json}")" + result_count="$(jq '.results | length' <<< "${search_json}")" + if (( result_count != 1 )); then + echo "Expected exactly one Artifactory match for ${wheel_name}, got ${result_count}" + echo "${search_json}" + exit 1 + fi + storage_uri="$(jq -r '.results[0].uri' <<< "${search_json}")"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 301 - 312, The current code reads search_json and unconditionally uses .results[0] to set storage_uri, which is unsafe when Artifactory returns multiple matches; update the logic around search_json/storage_uri to first count results (e.g., via jq '.results | length') and assert it equals 1, and if not log an explicit error with the full search_json and wheel_name and exit non‑zero; only when the count is exactly 1 extract .results[0].uri into storage_uri (and treat empty as an error as before). Ensure you reference the variables search_json, storage_uri and wheel_name and the jq-based results length check so the new guard replaces the blind .results[0] usage.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 445-447: The workflow step currently injects ARTIFACTORY_URL and
ARTIFACTORY_REPO into the environment but they are not referenced anywhere in
the step logic; remove these two environment variables from the env block (or
alternatively use them where needed) to avoid adding unused masked secrets—look
for the ARTIFACTORY_URL and ARTIFACTORY_REPO entries in the workflow step and
either delete those keys from the env map or update the step to actually consume
them.
---
Duplicate comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 361-364: The kitmaker job rebuilds artifact URLs from
ARTIFACTORY_URL and ARTIFACTORY_REPO instead of using the verified downloadUri
produced by publish-wheel, which breaks when secrets differ between
environments; modify the publish-wheel job to output the full downloadUri (or a
verified base URL) as a workflow output (e.g., add an output like
full_download_uris) and change the kitmaker job to consume that output (instead
of reconstructing with ARTIFACTORY_URL/ARTIFACTORY_REPO) by reading
needs.publish-wheel.outputs.full_download_uris (or the chosen name) and using
that value when setting WHEEL_PATHS/KITMAKER_UPLOAD payloads so the exact
validated URL from publish-wheel is passed across jobs.
- Around line 301-312: The current code reads search_json and unconditionally
uses .results[0] to set storage_uri, which is unsafe when Artifactory returns
multiple matches; update the logic around search_json/storage_uri to first count
results (e.g., via jq '.results | length') and assert it equals 1, and if not
log an explicit error with the full search_json and wheel_name and exit
non‑zero; only when the count is exactly 1 extract .results[0].uri into
storage_uri (and treat empty as an error as before). Ensure you reference the
variables search_json, storage_uri and wheel_name and the jq-based results
length check so the new guard replaces the blind .results[0] usage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: b93e6248-b9e7-4903-b926-83321cd4be9c
📒 Files selected for processing (1)
.github/workflows/build-ubuntu.yml
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.github/workflows/build-ubuntu.yml (3)
254-259:⚠️ Potential issue | 🟠 MajorMask Artifactory values before printing Artifactory responses.
Artifactory API responses printed on failure paths (lines 310, 317, 328, 336) can leak the Artifactory URL and repository name embedded in response URIs. Add masking after the URL validation:
if [[ "${ARTIFACTORY_URL}" != https://* ]]; then echo "ARTIFACTORY_URL must use https://" exit 1 fi + + echo "::add-mask::${ARTIFACTORY_URL}" + echo "::add-mask::${ARTIFACTORY_URL%/}" + echo "::add-mask::${ARTIFACTORY_REPO}" shopt -s nullglob🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 254 - 259, The workflow prints raw Artifactory API responses on failure which can expose ARTIFACTORY_URL and ARTIFACTORY_REPO; update the run step logic inside the same run block to mask these values before printing by replacing any occurrences of the ARTIFACTORY_URL and ARTIFACTORY_REPO environment variables in the response strings with masked placeholders (e.g., "***"); do this masking immediately after you validate or read the URL (the validation step mentioned in the comment) and before any echo/printf of responses on the failure paths so all failure logs use the sanitized response.
363-370:⚠️ Potential issue | 🟠 MajorDo not log the raw Kitmaker create-release response.
Line 426 echoes
response_jsonbefore any values are masked. The API response contains the submitted Artifactory URLs and therelease_uuid, which are then treated as sensitive in downstream steps. Add the mask block before the POST request and replace the raw response dump with a minimal sanitized log line.🧯 Safer logging pattern
if [[ "${ARTIFACTORY_URL}" != https://* ]]; then echo "ARTIFACTORY_URL must use https://" exit 1 fi + + echo "::add-mask::${KITMAKER_API_ENDPOINT}" + echo "::add-mask::${KITMAKER_API_ENDPOINT%/}" + echo "::add-mask::${KITMAKER_PROJECT_ID}" + echo "::add-mask::${ARTIFACTORY_URL}" + echo "::add-mask::${ARTIFACTORY_URL%/}" + echo "::add-mask::${ARTIFACTORY_REPO}" if [[ -z "${WHEEL_PATHS}" ]]; then echo "No wheel paths were produced by publish-wheel job" exit 1 fi @@ - echo "${response_json}" - release_uuid="$(jq -r '.release_uuid // empty' <<< "${response_json}")" if [[ -z "${release_uuid}" ]]; then echo "Kitmaker response missing release_uuid" exit 1 fi + echo "::add-mask::${release_uuid}" + echo "Kitmaker release created"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 363 - 370, The workflow currently echoes the raw response_json from the Kitmaker create-release POST and leaks Artifactory URLs and release_uuid; move or add the mask step (using ::add-mask or the repo's mask mechanism) before making the POST in the create-release step, stop echoing the full response_json after the curl/POST, and replace that dump with a minimal sanitized log (e.g., success/failure and sanitized release id placeholder). Update references in the create-release step to use the masked variables and ensure any downstream steps rely on the masked outputs rather than printing response_json.
224-233:⚠️ Potential issue | 🟠 MajorAdd PR guard to
publish-wheeljob for consistency with downstream jobs.The downstream
kitmakerandkitmaker-statusjobs both explicitly exclude pull requests withgithub.event_name != 'pull_request', butpublish-wheeldoes not. This allows same-repo PRs to upload unmerged wheels to the internal Artifactory repository. Fork PRs will fail at the secret check, but the asymmetry suggests unintended behavior. Gatepublish-wheelwith the same PR condition as downstream jobs.Suggested guard
- if: ${{ needs.build-ubuntu.result == 'success' && needs.test-cloudxr.result == 'success' }} + if: ${{ github.event_name != 'pull_request' && needs.build-ubuntu.result == 'success' && needs.test-cloudxr.result == 'success' }}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/build-ubuntu.yml around lines 224 - 233, The publish-wheel job currently lacks the PR guard used by downstream jobs (kitmaker and kitmaker-status), allowing same-repo pull requests to upload unmerged wheels; update the publish-wheel job (job name: publish-wheel) to include the same PR gate by adding the condition that github.event_name != 'pull_request' (the same check used in kitmaker / kitmaker-status) to its if expression so it only runs for non-PR events and matches downstream behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In @.github/workflows/build-ubuntu.yml:
- Around line 254-259: The workflow prints raw Artifactory API responses on
failure which can expose ARTIFACTORY_URL and ARTIFACTORY_REPO; update the run
step logic inside the same run block to mask these values before printing by
replacing any occurrences of the ARTIFACTORY_URL and ARTIFACTORY_REPO
environment variables in the response strings with masked placeholders (e.g.,
"***"); do this masking immediately after you validate or read the URL (the
validation step mentioned in the comment) and before any echo/printf of
responses on the failure paths so all failure logs use the sanitized response.
- Around line 363-370: The workflow currently echoes the raw response_json from
the Kitmaker create-release POST and leaks Artifactory URLs and release_uuid;
move or add the mask step (using ::add-mask or the repo's mask mechanism) before
making the POST in the create-release step, stop echoing the full response_json
after the curl/POST, and replace that dump with a minimal sanitized log (e.g.,
success/failure and sanitized release id placeholder). Update references in the
create-release step to use the masked variables and ensure any downstream steps
rely on the masked outputs rather than printing response_json.
- Around line 224-233: The publish-wheel job currently lacks the PR guard used
by downstream jobs (kitmaker and kitmaker-status), allowing same-repo pull
requests to upload unmerged wheels; update the publish-wheel job (job name:
publish-wheel) to include the same PR gate by adding the condition that
github.event_name != 'pull_request' (the same check used in kitmaker /
kitmaker-status) to its if expression so it only runs for non-PR events and
matches downstream behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 74104790-0932-4d6e-8b9e-12b3042140e2
📒 Files selected for processing (1)
.github/workflows/build-ubuntu.yml
Summary by CodeRabbit