Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: Adding CRD validation #888

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Conversation

Santosh1176
Copy link
Contributor

@Santosh1176 Santosh1176 commented Sep 6, 2022

Signed-off-by: Santosh Kaluskar [email protected]

This WIP is a part fix to #2993.
Request a review from @pjbgf.

Signed-off-by: Santosh Kaluskar <[email protected]>
Comment on lines 81 to 83
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:Pattern=`^([a-zA-Z0-9_\-.\\\/]|\[[0-9]{1,5}\])+$`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The ignore field can have thousands of lines with special characters and UTF8, think of file paths written in Chinese.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, Should the validation be removed from this Igonore?

Copy link
Member

@stefanprodan stefanprodan Sep 6, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The validation pattern yes, the max length should be less than etcd entry max size (1MB).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would that be 5000? (<5120)

api/v1beta1/bucket_types.go Outdated Show resolved Hide resolved
// +optional
Tag string `json:"tag,omitempty"`

// The Git tag semver expression, takes precedence over Tag.
// +kubebuilder:validation:MaxLength=253
// +kubebuilder:validation:Pattern=`^[\-._0-9]+$`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A semver expression usually contains special characters and alpha numeric ones, see https://github.com/Masterminds/semver

@@ -102,6 +110,9 @@ type LocalHelmChartSourceReference struct {
Kind string `json:"kind"`

// Name of the referent.
// +kubebuilder:validation:MinLength=1
// +kubebuilder:validation:MaxLength=63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:MaxLength=253

Kubernetes object name max length is 253, please use this value for all fields that references a Kube object by name.

// +required
URL string `json:"url"`

// Revision is a human-readable identifier traceable in the origin source
// system. It can be a Git commit SHA, Git tag, a Helm chart version, etc.
// +kubebuilder:validation:MaxLength=63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This breaks Flux, we use SHA256 for OCI and for Git we append the brach name to the Git SHA.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@@ -92,6 +97,9 @@ const (
// the typed referenced object at namespace level.
type LocalHelmChartSourceReference struct {
// APIVersion of the referent.
// +kubebuilder:validation:MaxLength=63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:MaxLength=2048

Please change this in all objects with APIVersion fields.

@@ -153,14 +161,20 @@ type GitRepositoryRef struct {
//
// When GitRepositorySpec.GitImplementation is set to 'go-git', a shallow
// clone of the specified branch is performed.
// +kubebuilder:validation:MaxLength=63
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:MaxLength=244

Comment on lines 44 to 45
// +kubebuilder:validation:MaxLength=63
// +kubebuilder:validation:Pattern=`^[\-._0-9]+$`
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The version can be a semver expression as documented above

@stefanprodan
Copy link
Member

@Santosh1176 please run make generate manifests api-docs to generate the CRD YAMLs, then run make verify, once verify no longer errors out, please commit all changes.

@Santosh1176
Copy link
Contributor Author

@Santosh1176 please run make generate manifests api-docs to generate the CRD YAMLs, then run make verify, once verify no longer errors out, please commit all changes.

Thanks for the review and your time. I'll do as advised.

Signed-off-by: Santosh Kaluskar <[email protected]>
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.

3 participants