fix(crd): validate packaged CRD artifacts - #202
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1cc8813c6a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| .map(|crd| crd.metadata.name.expect("generated CRD should have a name")) | ||
| .collect::<BTreeSet<_>>(); | ||
| let crd_dir = PathBuf::from(env!("CARGO_MANIFEST_DIR")).join("deploy/rustfs-operator/crds"); | ||
| let mut entries = fs::read_dir(&crd_dir) |
There was a problem hiding this comment.
When a CRD is placed under a subdirectory such as crds/vendor/foo.yaml, fs::read_dir returns only the vendor directory, which the extension filter then skips, so this validation passes without checking that packaged artifact. Helm’s CRDObjects implementation selects chart files by the crds/ path prefix, including nested files; recursively walk this directory or inspect the packaged chart so duplicate and unexpected nested CRDs are rejected.
Useful? React with 👍 / 👎.
| for crd in parse_crds(&yaml) { | ||
| let name = crd.metadata.name.as_deref().unwrap_or("<unnamed>"); | ||
| assert!( | ||
| crd.spec.names.categories.is_none(), |
There was a problem hiding this comment.
If rendering regresses to emitting spec.names.categories: null, deserializing into the optional categories field produces None, so this assertion passes even though the key was not omitted. That allows regenerated packaged artifacts to violate the exact omission this regression test is intended to enforce; inspect the YAML mapping for key presence rather than relying only on the deserialized Option.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5cb09cf533
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ["yaml", "yml", "json"] | ||
| .iter() | ||
| .any(|expected| extension.eq_ignore_ascii_case(expected)) |
There was a problem hiding this comment.
When a file under crds/ uses an uppercase suffix—as the new .YML and .JSON fixtures do—this helper selects it, but Helm's hasManifestExtension accepts only the exact lowercase extensions .yaml, .yml, and .json. The validation therefore does not actually mirror Helm: it can parse and count an artifact that Helm will skip during CRD installation, or fail on an uppercase auxiliary file Helm ignores. Use the same case-sensitive extension comparison as Helm.
Useful? React with 👍 / 👎.
1 participant
Type of Change
Related Issues
Follow-up to #198.
Summary of Changes
spec.names.categories.Checklist
make pre-commit(fmt-check + clippy + test + console-lint + console-fmt-check)[Unreleased](N/A; no user-facing feature change)Impact
Verification
Additional Notes
This addresses the review follow-ups identified after merging #198.