feat: add ModelArtifact CRD and related API definitions - #24
Conversation
iasthc
commented
Feb 28, 2026
iasthc
commented
- Introduced the ModelArtifact Custom Resource Definition (CRD) to manage model artifacts.
- Added API schema definitions for ModelArtifact, including specifications for source, target, format, and storage.
- Updated kustomization.yaml to include the new ModelArtifact CRD resource.
- Implemented deepcopy functions for the new API types to ensure compatibility.
- Introduced the ModelArtifact Custom Resource Definition (CRD) to manage model artifacts. - Added API schema definitions for ModelArtifact, including specifications for source, target, format, and storage. - Updated kustomization.yaml to include the new ModelArtifact CRD resource. - Implemented deepcopy functions for the new API types to ensure compatibility.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a new Highlights
Changelog
Activity
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR introduces a new model.otterscale.io/v1alpha1 API group with a ModelArtifact CustomResourceDefinition to represent importing a model from a source, packaging it as an OCI artifact, and pushing it to a registry.
Changes:
- Added
ModelArtifactAPI types (spec/status) and group registration formodel.otterscale.io/v1alpha1. - Generated deepcopy implementations for the new API types.
- Added the generated
ModelArtifactCRD to the CRD kustomization resources.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
model/v1alpha1/modelartifact_types.go |
Defines the ModelArtifact CRD Go types (spec/status, source/target/storage structs, enums). |
model/v1alpha1/groupversion_info.go |
Registers ModelArtifact and ModelArtifactList with the scheme for the new API group. |
model/v1alpha1/zz_generated.deepcopy.go |
Controller-gen generated deepcopy methods for the new types. |
config/crd/bases/model.otterscale.io_modelartifacts.yaml |
Generated CRD manifest for ModelArtifact including schema and printer columns. |
config/crd/kustomization.yaml |
Includes the new CRD base in the CRD kustomization. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| Name string `json:"name"` | ||
|
|
||
| // Key is the key within the Secret data. If omitted, defaults to "token". | ||
| // +optional |
There was a problem hiding this comment.
The key field comment says it defaults to "token", but the CRD does not specify a default and there is no defaulting webhook. This makes the API documentation inaccurate and forces every user to set the key explicitly. Consider adding +kubebuilder:default=token or updating the comment and handling empty string in the controller.
| // +optional | |
| // +optional | |
| // +kubebuilder:default=token |
| // ModelSource defines the origin of the model to be packaged. | ||
| // Exactly one source type must be specified. | ||
| // +kubebuilder:validation:XValidation:rule="has(self.huggingFace)",message="at least one source must be specified" | ||
| type ModelSource struct { | ||
| // HuggingFace specifies a HuggingFace Hub repository as the model source. | ||
| // +optional | ||
| HuggingFace *HuggingFaceSource `json:"huggingFace,omitempty"` | ||
| } |
There was a problem hiding this comment.
ModelSource is documented as "Exactly one source type must be specified", but the current validation rule/message only enforces that huggingFace is present ("at least one source"). Please align the docs and validation (e.g., change the comment to "At least one source must be specified" for now, or implement an XOR-style validation if/when multiple sources are supported).
| ObservedGeneration int64 `json:"observedGeneration,omitempty"` | ||
|
|
||
| // Phase is the high-level summary of the artifact lifecycle. | ||
| // +optional |
There was a problem hiding this comment.
status.phase is typed as ArtifactPhase, but the generated CRD schema does not restrict it to the allowed values (Pending/Running/Succeeded/Failed). If clients can set arbitrary strings, it weakens the contract and makes kubectl output/UX less predictable. Add a +kubebuilder:validation:Enum=Pending;Running;Succeeded;Failed marker on the Phase field (or equivalent) so the CRD enforces the enum.
| // +optional | |
| // +optional | |
| // +kubebuilder:validation:Enum=Pending;Running;Succeeded;Failed |
| Repository string `json:"repository"` | ||
|
|
||
| // Tag is the image tag to push. Defaults to "latest" if not specified. | ||
| // +kubebuilder:validation:MaxLength=128 |
There was a problem hiding this comment.
The tag field comment states it defaults to "latest", but there is no CRD default (no +kubebuilder:default=latest) and no defaulting webhook in the repo. As-is, omitting spec.target.tag results in an empty string, which contradicts the API docs and may break consumers expecting "latest". Either add a kubebuilder default for this field or update the comment and ensure the controller handles the empty-string case explicitly.
| // +kubebuilder:validation:MaxLength=128 | |
| // +kubebuilder:validation:MaxLength=128 | |
| // +kubebuilder:default=latest |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request introduces a new ModelArtifact Custom Resource Definition (CRD) and its associated API types. The changes are well-structured and include the CRD YAML, Go type definitions, and updates to the build configuration. My review found a minor but important issue in the Go type definitions where an incorrect JSON tag omitzero was used instead of omitempty. This could lead to incorrect JSON serialization. I have provided suggestions to correct this. With these changes, the PR will be in great shape.
|
|
||
| // Standard object's metadata. | ||
| // +optional | ||
| metav1.ObjectMeta `json:"metadata,omitzero"` |
There was a problem hiding this comment.
The JSON tag omitzero is not a valid option in Go's encoding/json package. This should be omitempty to ensure that this field is omitted from the JSON output when it is empty. Using an incorrect tag can lead to unexpected serialization behavior.
| metav1.ObjectMeta `json:"metadata,omitzero"` | |
| metav1.ObjectMeta `json:"metadata,omitempty"` |
|
|
||
| // Status represents the current state of the model artifact pipeline. | ||
| // +optional | ||
| Status ModelArtifactStatus `json:"status,omitzero"` |
There was a problem hiding this comment.
The JSON tag omitzero is not a valid option. It should be omitempty.
| Status ModelArtifactStatus `json:"status,omitzero"` | |
| Status ModelArtifactStatus `json:"status,omitempty"` |
| // ModelArtifactList contains a list of ModelArtifact resources. | ||
| type ModelArtifactList struct { | ||
| metav1.TypeMeta `json:",inline"` | ||
| metav1.ListMeta `json:"metadata,omitzero"` |
There was a problem hiding this comment.
The JSON tag omitzero is not a valid option. It should be omitempty.
| metav1.ListMeta `json:"metadata,omitzero"` | |
| metav1.ListMeta `json:"metadata,omitempty"` |
