Conversation
Signed-off-by: Alan Clucas <alan@clucas.org>
73ed4d2 to
e9c207a
Compare
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR makes breaking changes to the Argo Workflows API to improve flexibility and correctness:
- Makes
PluginArtifactfieldsnameandconfigurationoptional instead of required - Changes
Cache.ConfigMapfromConfigMapKeySelectortoLocalObjectReference(removing the unusedkeyfield) - Adds
+kubebuilder:pruning:PreserveUnknownFieldsdirective toPlugintype to allow arbitrary plugin configurations - Adds comprehensive CRD validation tests for all example and test YAML files
- Updates test data and examples to comply with the new schemas
Reviewed Changes
Copilot reviewed 47 out of 49 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
pkg/apis/workflow/v1alpha1/workflow_types.go |
Made PluginArtifact.Name and Configuration optional; changed Cache.ConfigMap to LocalObjectReference |
pkg/apis/workflow/v1alpha1/plugin_types.go |
Added PreserveUnknownFields directive to Plugin type |
pkg/apis/workflow/v1alpha1/crd_examples_test.go |
New comprehensive test to validate all examples against CRD schemas |
test/e2e/testdata/*.yaml |
Fixed test files to comply with updated schemas (added required fields, converted integer params to strings) |
examples/*.yaml |
Updated examples to remove unused ConfigMap key field and fix parameter types |
manifests/base/crds/ |
Regenerated CRD manifests reflecting API changes |
sdks/ |
Regenerated SDK documentation and code for Python and Java clients |
go.mod, go.sum |
Added dependencies for CRD validation (apiextensions-apiserver, cel-go, etc.) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
isubasinghe
approved these changes
Nov 11, 2025
guanguxiansheng
pushed a commit
to guanguxiansheng/argo-workflows
that referenced
this pull request
Dec 15, 2025
Signed-off-by: Alan Clucas <alan@clucas.org>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
To complement #15003
The examples validation test didn't catch everything this one did, and doesn't test
test/e2ebut is someways overlapping. Neither take long to run.Motivation
We have quite a lot of example YAML files - so lets prove that our full CRDs don't violate them.
Will be followed by an update to the CRDs to add some CEL validation (kubebuilder rules) for validation rules that can easily happen at that level.
Modifications
Add pkg/apis/workflow/v1alpha1/crd_examples_test.go which runs through all
/examplesYAML files and most of them in test/e2e (ignoring some which are marked as invalid). This file already does CEL validation tests, but doesn't introduce CEL rules.It performs strict validation.
What did this discover and get fixed here:
ConfigMapKeySelectorfor memoizationCacheis just the name of the ConfigMap, the key is in the parent object so we only want this to be theLocalObjectReferenceto find the ConfigMap. There are no other code changes around this, which validates this is the correct fix.ArtifactPluginsdid have required name and configuration. Configuration is "obviously" optional. Name is also optional in the case where we're just specifying a key and using a pre-configured artifact repository. Made bothomitemptyto make them optional.[]->"[]"and9->"9"labels:->labels: {}artifactPaths(which is weirdly plural when the content is singular) only contain artifactLocation (+maybe some other fields) asNameisn't important here. I tried fixing this and it got out of hand, and this is rarely used I believe, so left it for another time.Verification
This is a new test.
Documentation
None here.