Conversation
…validation - Added 'registry' field to the ModelArtifact CRD for specifying the OCI registry host. - Renamed 'repository' field in HuggingFaceSource to 'model' for clarity. - Updated descriptions and validation patterns for 'repository' and 'model' fields to enhance understanding and enforce naming conventions. - Adjusted required fields in the CRD to include the new 'registry' field.
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 refines the 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
|
![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 registry field to the ModelArtifact CRD, separating it from the repository path for better clarity and validation. It also improves the HuggingFace source definition by renaming repository to model. The changes are well-structured and improve the API. I've found one issue with the validation pattern for the new registry field, which is too permissive and allows invalid characters for a registry host.
| port (e.g. "ghcr.io", "registry.local:5001"). | ||
| maxLength: 253 | ||
| minLength: 1 | ||
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$ |
There was a problem hiding this comment.
The validation pattern for registry allows slashes (/), which are not valid in a registry hostname. A registry host should be a DNS name or IP, optionally with a port, but should not contain path segments. Please remove the slash from the pattern to enforce a valid registry host format.
pattern: ^[a-zA-Z0-9][a-zA-Z0-9._:-]*$| // Registry is the OCI registry host, optionally with port (e.g. "ghcr.io", "registry.local:5001"). | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$" |
There was a problem hiding this comment.
The validation pattern for Registry allows slashes (/), which are not valid in a registry hostname. A registry host should be a DNS name or IP, optionally with a port, but should not contain path segments. Please remove the slash from the pattern to enforce a valid registry host format. This will also fix the generated CRD YAML.
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$" | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._:-]*$" |
There was a problem hiding this comment.
Pull request overview
Updates the ModelArtifact Kubernetes API (Go types + generated CRD) to better structure OCI destination information and clarify HuggingFace source naming/validation.
Changes:
- Add
.spec.target.registryand expose it as an additional printer column. - Rename HuggingFace source field from
repositorytomodeland update schema/required fields accordingly. - Refine descriptions and validation patterns for the updated fields.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
model/v1alpha1/modelartifact_types.go |
Adds registry to OCITarget, renames HuggingFace repository to model, and updates printer columns. |
config/crd/bases/model.otterscale.io_modelartifacts.yaml |
Regenerates CRD schema/printer columns to reflect the API changes and required fields. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| model: | ||
| description: |- | ||
| Repository is the HuggingFace model repository identifier (e.g. "microsoft/phi-4"). | ||
| Model is the HuggingFace model identifier (e.g. "microsoft/phi-4", "facebook/opt-125m"). | ||
| Must contain only alphanumerics, dots, underscores, hyphens, and slashes. | ||
| maxLength: 253 |
There was a problem hiding this comment.
Renaming the HuggingFace field from repository to model is a breaking CRD change: existing manifests using spec.source.huggingFace.repository will fail validation (and the unknown field may be pruned on write). If upgrades need to be supported, consider keeping repository as a deprecated alias (with validation to ensure only one is set) or providing a conversion/migration path.
| pattern: ^[a-zA-Z0-9._/-]*$ | ||
| type: string | ||
| required: | ||
| - registry |
There was a problem hiding this comment.
Making .spec.target.registry required is an API-breaking change for any existing ModelArtifacts that only set repository (as a full ref previously). If cluster upgrades must be smooth, consider temporarily allowing registry to be optional with defaulting/derivation from repository, or supporting both forms during a deprecation window.
| - registry |
| // Registry is the OCI registry host, optionally with port (e.g. "ghcr.io", "registry.local:5001"). | ||
| // +kubebuilder:validation:MinLength=1 | ||
| // +kubebuilder:validation:MaxLength=253 | ||
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$" |
There was a problem hiding this comment.
The Registry field is documented as an OCI registry host (optionally with port), but the validation pattern currently allows /. That permits path segments (e.g. ghcr.io/myorg), which contradicts the field’s intent and undermines the separation between registry and repository. Tighten the regex to disallow / (and ideally validate host[:port]).
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$" | |
| // +kubebuilder:validation:Pattern="^[a-zA-Z0-9][a-zA-Z0-9._-]*(?::[0-9]{1,5})?$" |
| port (e.g. "ghcr.io", "registry.local:5001"). | ||
| maxLength: 253 | ||
| minLength: 1 | ||
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$ |
There was a problem hiding this comment.
The CRD schema for .spec.target.registry uses a pattern that allows /, even though registry is described as a host[:port]. This permits values like ghcr.io/myorg and blurs the boundary with .spec.target.repository. Update the regex to reject / and more closely match a registry host (optionally with port).
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._/:-]*$ | |
| pattern: ^[a-zA-Z0-9][a-zA-Z0-9._-]*(?::[0-9]{1,5})?$ |
| description: HuggingFace specifies a HuggingFace Hub repository | ||
| as the model source. |
There was a problem hiding this comment.
The huggingFace source description still says it specifies a HuggingFace Hub "repository", but the field has been renamed to model. Update this text to avoid confusion for CRD users reading the generated schema.
| description: HuggingFace specifies a HuggingFace Hub repository | |
| as the model source. | |
| description: HuggingFace specifies a HuggingFace Hub model | |
| (via the model identifier) as the model source. |
…validation