Skip to content

fix(controller): skip empty optional step artifacts - #16839

Merged
terrytangyuan merged 1 commit into
argoproj:mainfrom
rwong2888:fix/optional-skipped-step-artifact
Aug 31, 2026
Merged

fix(controller): skip empty optional step artifacts#16839
terrytangyuan merged 1 commit into
argoproj:mainfrom
rwong2888:fix/optional-skipped-step-artifact

Conversation

@rwong2888

@rwong2888 rwong2888 commented Aug 28, 2026

Copy link
Copy Markdown
Contributor
  • Ran make pre-commit -B
  • Signed-off commits with Conventional Commit messages
  • PR title is a conventional commit message (it becomes the release notes entry)
  • Unit or e2e tests cover the change
  • For features: an associated issue and a feature description file (make feature-new)
  • Opened as draft; will mark "Ready for review" once builds are green

Fixes #16477

Motivation

A running step that consumes an optional: true artifact from a skipped step can retain an empty artifact placeholder. Later PodSpecPatch substitution then attempts to resolve that placeholder and fails.

Modifications

Drop optional step artifact arguments that resolve without an artifact location or key. Add a controller regression test covering an optional artifact resolved from a skipped step output.

Verification

Ran:

KUBECONFIG=/dev/null go test -run 'Test(OptionalArgumentAndParameter|ResolveReferencesSkipsOptionalArtifactFromSkippedStep)' ./workflow/controller/

I also built the controller and argoexec and updated the controller's image and the configmap'as argoexec image.

I reran the workflow in the issue and it passed.

image

Documentation

No documentation change is needed. This restores the documented behavior of optional artifact inputs.

AI

GitHub Copilot assisted with investigation, implementation, test creation, and this PR description.

Summary by CodeRabbit

  • Bug Fixes
    • Optional artifacts without available data are no longer passed to workflow steps.
    • Steps depending on skipped optional artifacts now proceed without invalid artifact arguments.
  • Tests
    • Added coverage to verify optional artifacts from skipped steps are correctly omitted.

Signed-off-by: rwong2888 <rwong@forbes.com>
@github-actions github-actions Bot added the problem/bot-not-ready Readiness bot declares this as not ready, see comment by bot for why label Aug 28, 2026
@github-actions

github-actions Bot commented Aug 28, 2026

Copy link
Copy Markdown
Contributor

✅ PR readiness: all clear

All contributor-fixable checks are passing. A maintainer will take it from here — thanks!


🤖 Automated PR-readiness helper — it re-checks each time CI finishes. Unit/E2E test results are not covered here. Questions? See the contributing guide or ask a maintainer.

@codecov

codecov Bot commented Aug 28, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 39.92%. Comparing base (79ef17f) to head (ff11a6e).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16839      +/-   ##
==========================================
- Coverage   39.95%   39.92%   -0.04%     
==========================================
  Files         569      569              
  Lines       44724    44726       +2     
==========================================
- Hits        17869    17855      -14     
- Misses      25074    25086      +12     
- Partials     1781     1785       +4     
Flag Coverage Δ
unit-tests 39.90% <100.00%> (-0.04%) ⬇️
unit-tests-windows 28.79% <ø> (-0.01%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions Bot removed the problem/bot-not-ready Readiness bot declares this as not ready, see comment by bot for why label Aug 28, 2026
@rwong2888
rwong2888 marked this pull request as ready for review August 31, 2026 19:50
@rwong2888
rwong2888 requested a review from a team as a code owner August 31, 2026 19:50
@coderabbitai

coderabbitai Bot commented Aug 31, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 57c43893-8096-4ce2-b2d2-d1426ad7319b

📥 Commits

Reviewing files that changed from the base of the PR and between bde5adf and ff11a6e.

📒 Files selected for processing (2)
  • workflow/controller/steps.go
  • workflow/controller/steps_test.go

Included review availability: Your plan provides up to 10 included reviews per hour; 9 remain after this review.


📝 Walkthrough

Walkthrough

resolveReferences now omits optional artifact arguments when resolution produces no location or key. A test covers an optional artifact referencing output from a skipped step.

Changes

Optional artifact resolution

Layer / File(s) Summary
Skip empty optional artifacts
workflow/controller/steps.go, workflow/controller/steps_test.go
resolveReferences omits optional artifacts without a location or key. The test verifies that an artifact from a skipped source step is removed from the step arguments.

Estimated code review effort: 2 (Simple) | ~10 minutes

Merge Risk: ⚪ Minimal · up to ff11a

The controller now omits empty optional artifact placeholders and includes regression coverage for skipped-step outputs. No actionable merge-blocking risk remains after normal checks and review.

Suggested reviewers: joibel, isubasinghe

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main controller change: skipping empty optional step artifacts.
Description check ✅ Passed The description includes the required checklist, linked issue, motivation, modifications, verification, documentation rationale, and AI disclosure. It provides sufficient detail for review.
Linked Issues check ✅ Passed The implementation and regression test address issue #16477 by dropping optional artifact arguments that resolve without a location or key when the producing step is skipped.
Out of Scope Changes check ✅ Passed The changes are limited to the controller behavior and its regression test. They directly support the linked issue and stated objectives.
  • Fix all pre-merge checks with AI
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with 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.

❤️ Share

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

@terrytangyuan
terrytangyuan merged commit 2434a0c into argoproj:main Aug 31, 2026
55 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Optional artifacts are not respected when the producing step is skipped

2 participants