Skip to content

docs: explain argocd-helm OCI credential matching#1070

Merged
mchmarny merged 2 commits into
NVIDIA:mainfrom
kiwigitops:docs-argocd-oci-credentials
May 31, 2026
Merged

docs: explain argocd-helm OCI credential matching#1070
mchmarny merged 2 commits into
NVIDIA:mainfrom
kiwigitops:docs-argocd-oci-credentials

Conversation

@kiwigitops
Copy link
Copy Markdown
Contributor

Summary

  • document the Argo CD repo-credential URL matching rule for argocd-helm OCI bundles
  • call out the private-registry 401 case where local Helm auth succeeds but Argo repo-server credentials are keyed to a different source URL

Related #1049.

Testing

  • git diff --check

@kiwigitops kiwigitops requested a review from a team as a code owner May 28, 2026 01:53
@copy-pr-bot
Copy link
Copy Markdown

copy-pr-bot Bot commented May 28, 2026

This pull request requires additional validation before any workflows can run on NVIDIA's runners.

Pull request vetters can view their responsibilities here.

Contributors can view more details about this message here.

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to AICR, @kiwigitops! Thanks for your first pull request.

Before review, please ensure:

  • All commits are signed off per the DCO
  • CI checks pass (tests, lint, security scan)
  • The PR description explains the why behind your changes

A maintainer will review this soon.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Updates docs/user/cli-reference.md for the aicr bundle --deployer argocd-helm section: clarifies that parent Applications append .Chart.Name into OCI source.repoURL while path-based child Applications append the chart name to the rendered source.repoURL (so including the chart in --set repoURL causes double-appending); adds a troubleshooting bullet explaining that for path-based OCI Applications Argo CD repo-server resolves sources as oci:////, and private registry credentials must be keyed to that rendered OCI prefix (or a broader permitted prefix) to avoid auth/fetch failures.

Estimated code review effort

🎯 1 (Trivial) | ⏱️ ~3 minutes

Possibly related PRs

  • NVIDIA/aicr#1032: Related CLI-reference clarification around repoURL/chart-name handling for argocd-helm.
  • NVIDIA/aicr#1035: Code changes that append {{ .Chart.Name }} for path-based child Applications, matching the CLI docs' contract.
  • NVIDIA/aicr#1039: Related fixes and discussion about repoURL rendering and credential matching for path-based OCI Applications.

Suggested reviewers

  • mchmarny
  • yuanchen8911
🚥 Pre-merge checks | ✅ 4
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title directly describes the main documentation change: explaining Argo CD repo-credential URL matching for argocd-helm OCI bundles, which aligns with the file changes and PR objectives.
Description check ✅ Passed The description is related to the changeset, outlining the two main documentation additions: OCI credential matching rules and the private-registry 401 failure case with solutions.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands and usage tips.

mchmarny
mchmarny previously approved these changes May 28, 2026
@github-actions
Copy link
Copy Markdown
Contributor

@mchmarny
Copy link
Copy Markdown
Member

@kiwigitops thanks for the doc update.
Looks like the 073272c97be438f64abb84f28ccf5a0335ae5864 commit is not signed. Please amend it. Otherwise LGTM

@mchmarny mchmarny self-assigned this May 28, 2026
@kiwigitops kiwigitops force-pushed the docs-argocd-oci-credentials branch from d42b571 to fdba133 Compare May 28, 2026 16:38
@kiwigitops
Copy link
Copy Markdown
Contributor Author

Updated with a signed-off commit. I also rebased onto current main while amending it.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/user/cli-reference.md`:
- Line 2489: Split the single dense troubleshooting paragraph titled "4.
**Private registry credentials keyed to a different source URL.**" into three
short paragraphs labeled Problem, Failure case, and Solution; keep the original
examples like `oci://<registry>/<path>/<chart>` and `--set repoURL` but make the
failure case explicit (Secret keyed only to `<registry>/<path>` or scheme-less
Helm-OCI URL causes Argo repo-server 401) and in the Solution paragraph show a
concrete example of a broader matching prefix (e.g., use `oci://<registry>/` or
`oci://<registry>/<path>/` as credential keys) and recommend choosing the
rendered `oci://.../<chart>` prefix or a cluster-allowed broader prefix.
🪄 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: 0245a7f1-df22-4030-b2e4-dfc8ece28e06

📥 Commits

Reviewing files that changed from the base of the PR and between 073272c and fdba133.

📒 Files selected for processing (1)
  • docs/user/cli-reference.md

Comment thread docs/user/cli-reference.md Outdated
@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 28, 2026

Actionable comments posted: 0

@kiwigitops
Copy link
Copy Markdown
Contributor Author

Updated the troubleshooting item into Problem / Failure case / Solution and kept the OCI credential-prefix examples. CodeRabbit is green on the follow-up commit, and the commits remain signed off.

mchmarny
mchmarny previously approved these changes May 28, 2026
@mchmarny mchmarny self-requested a review May 28, 2026 19:48
@mchmarny
Copy link
Copy Markdown
Member

mchmarny commented May 28, 2026

Updated with a signed-off commit.

@kiwigitops still seeing "Commits must have verified signatures." This is -S not -s on the git commit command (https://github.com/NVIDIA/aicr/pull/1070/commits)

@mchmarny mchmarny assigned kiwigitops and unassigned mchmarny May 28, 2026
Copy link
Copy Markdown
Contributor

@yuanchen8911 yuanchen8911 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm A couple of minor issues.

@yuanchen8911
Copy link
Copy Markdown
Contributor

Please rebase.

Comment thread docs/user/cli-reference.md
Comment thread docs/user/cli-reference.md
@kiwigitops
Copy link
Copy Markdown
Contributor Author

Updated again. This is now rebased onto current main as a single verified commit, with the two review notes folded in:

  • changed the section intro to Common causes:
  • corrected the OCI wording to source.repoURL in both places, while keeping the non-OCI source.chart distinction

The PR diff is back to one file with 12 additions / 6 deletions.

Copy link
Copy Markdown

@coderabbitai coderabbitai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 `@docs/user/cli-reference.md`:
- Around line 2589-2593: Update the section intro that currently reads "Three
common causes, in order of likelihood:" (locate that exact phrase in the same
section as the new fourth item) so it no longer misstates the count—change it to
either "Four common causes:" or a future-proofed "Common causes:" to reflect the
added fourth item about OCI registry credential keys (the new item beginning
"Private registry credentials keyed to a different source URL.").
🪄 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: 2bf1dd0b-2382-47d6-998d-ae80ab57bd12

📥 Commits

Reviewing files that changed from the base of the PR and between 0ef5536 and 5940120.

📒 Files selected for processing (1)
  • docs/user/cli-reference.md

Comment thread docs/user/cli-reference.md
@kiwigitops kiwigitops closed this May 30, 2026
@kiwigitops kiwigitops force-pushed the docs-argocd-oci-credentials branch from 5940120 to 1c82645 Compare May 30, 2026 19:54
@github-actions github-actions Bot added size/XS and removed size/S labels May 30, 2026
Signed-off-by: kiwigitops <kiwisclubco@gmail.com>
@kiwigitops kiwigitops reopened this May 30, 2026
@github-actions github-actions Bot added size/S and removed size/XS labels May 30, 2026
@mchmarny mchmarny merged commit b98d3d7 into NVIDIA:main May 31, 2026
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants