test(ci): probe velonix->Artifactory docker push reachability#1834
test(ci): probe velonix->Artifactory docker push reachability#1834nv-nmailhot wants to merge 2 commits into
Conversation
Throwaway workflow on a velonix runner that checks TCP 443 reachability, docker login, and a tiny image push to the SWX Artifactory registry. Registry base kept in a secret so the internal URL is masked. Delete after the reachability question is answered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds a GitHub Actions workflow for a single branch that verifies Artifactory TCP reachability, authenticates with registry credentials, builds a minimal container image, and pushes it to a run-specific Artifactory tag. ChangesArtifactory push test workflow
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![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
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/artifactory-push-test.yml:
- Around line 12-23: The artifactory push test workflow currently has no
concurrency control, so repeated pushes to the same branch can leave older runs
active and waste runner time. Add a workflow-level concurrency block to the
artifactory-push-test job workflow so new runs cancel any in-progress run for
the same branch, using the existing branch trigger and workflow name as the
grouping key.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: 63ba0503-6242-4512-8eef-8041c112af9c
📒 Files selected for processing (1)
.github/workflows/artifactory-push-test.yml
| on: | ||
| push: | ||
| branches: | ||
| - nmailhot/artifactory-reachability-test | ||
|
|
||
| permissions: | ||
| contents: read | ||
|
|
||
| jobs: | ||
| artifactory-push-test: | ||
| runs-on: ${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-builder-amd-v1 | ||
| timeout-minutes: 15 |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick win
Add workflow concurrency to cancel stale test runs.
This branch-only probe can be triggered repeatedly while earlier runs are still active, which just burns runner time and can leave redundant pushes behind. A small concurrency block is enough here.
Suggested workflow stanza
on:
push:
branches:
- nmailhot/artifactory-reachability-test
+
+concurrency:
+ group: artifactory-push-test-${{ github.ref }}
+ cancel-in-progress: true📝 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.
| on: | |
| push: | |
| branches: | |
| - nmailhot/artifactory-reachability-test | |
| permissions: | |
| contents: read | |
| jobs: | |
| artifactory-push-test: | |
| runs-on: ${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-builder-amd-v1 | |
| timeout-minutes: 15 | |
| on: | |
| push: | |
| branches: | |
| - nmailhot/artifactory-reachability-test | |
| concurrency: | |
| group: artifactory-push-test-${{ github.ref }} | |
| cancel-in-progress: true | |
| permissions: | |
| contents: read | |
| jobs: | |
| artifactory-push-test: | |
| runs-on: ${{ vars.NIXL_RUNNER_PREFIX || 'prod' }}-nixl-builder-amd-v1 | |
| timeout-minutes: 15 |
🧰 Tools
🪛 zizmor (1.26.1)
[info] 21-21: workflow or action definition without a name (anonymous-definition): this job
(anonymous-definition)
[warning] 12-15: insufficient job-level concurrency limits (concurrency-limits): workflow is missing concurrency setting
(concurrency-limits)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/artifactory-push-test.yml around lines 12 - 23, The
artifactory push test workflow currently has no concurrency control, so repeated
pushes to the same branch can leave older runs active and waste runner time. Add
a workflow-level concurrency block to the artifactory-push-test job workflow so
new runs cancel any in-progress run for the same branch, using the existing
branch trigger and workflow name as the grouping key.
Source: Linters/SAST tools
…_TOKEN Use the existing repo secrets (no new secrets needed) so the velonix->Artifactory push can be tested immediately: docker login as nmailhot, push a tiny image to sw-dynamo-nixl-docker-local. Same auth path PR3 uses. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.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 (1)
.github/workflows/artifactory-push-test.yml (1)
37-58: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winMask the derived registry host before Docker emits it.
The PR summary says the internal URL should stay masked, but
AF_HOSTis derived from the secret rather than read directly from it. GitHub only redacts exact secret values, sodocker login/docker pushcan still leak the internal host in logs once they print the registry reference.Suggested fix
AF_HOST="$(printf '%s' "$ARTIFACTORY_URL" | sed -E 's#^https?://##; s#/.*##')" + echo "::add-mask::$AF_HOST"🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In @.github/workflows/artifactory-push-test.yml around lines 37 - 58, The workflow step derives AF_HOST from ARTIFACTORY_URL, but GitHub masking won’t redact that computed registry host when docker login or docker push prints it. Update the artifactory-push-test job to explicitly mask the derived host before any Docker commands run, using the same step that computes AF_HOST, so docker emitted registry references stay hidden in logs.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In @.github/workflows/artifactory-push-test.yml:
- Around line 37-58: The workflow step derives AF_HOST from ARTIFACTORY_URL, but
GitHub masking won’t redact that computed registry host when docker login or
docker push prints it. Update the artifactory-push-test job to explicitly mask
the derived host before any Docker commands run, using the same step that
computes AF_HOST, so docker emitted registry references stay hidden in logs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Enterprise
Run ID: dcf63eb6-92e9-40e4-8c64-45de3d8b3515
📒 Files selected for processing (1)
.github/workflows/artifactory-push-test.yml
1 participant
Throwaway workflow on a velonix runner that checks TCP 443 reachability, docker login, and a tiny image push to the SWX Artifactory registry. Registry base kept in a secret so the internal URL is masked. Delete after the reachability question is answered.
What?
Describe what this PR is doing.
Why?
Justification for the PR. If there is an existing issue/bug, please reference it. For
bug fixes, the 'Why?' and 'What?' can be merged into a single item.
How?
It is optional, but for complex PRs, please provide information about the design,
architecture, approach, etc.
Summary by CodeRabbit