Skip to content

enhancement: Remove spec level global storage dependency when rack level storage is used#440

Open
tanmayja wants to merge 7 commits intomasterfrom
enhancement/KO-481-remove-global-storage-validation
Open

enhancement: Remove spec level global storage dependency when rack level storage is used#440
tanmayja wants to merge 7 commits intomasterfrom
enhancement/KO-481-remove-global-storage-validation

Conversation

@tanmayja
Copy link
Copy Markdown
Contributor

No description provided.

//
// Note: revision character validity (IsDNS1123Label) is checked in
// validateRackConfig, which runs on both CREATE and UPDATE, because an
// invalid character would silently corrupt the StatefulSet name at runtime.
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

nit: These comments and comments on the func definition are kind of same/duplicate. Let's keep only the required comments here

Comment on lines +150 to +154
return fmt.Errorf(
"cluster %q: pod name %q (computed at max rack ID %d, revision length %d, max pod ordinal %d) "+
"exceeds the maximum DNS label length of %d characters; "+
"shorten the cluster name or rack revision",
cluster.Name, worstCasePodName, asdbv1.MaxRackID, maxPlaceholderLen,
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

I'm not sure if adding info like worstCasePodName with "aaa", "1000000" and "255" in error response will be useful for the user. I think this will confuse the user more as none of these values are inputs for the user.
Can we rephrase in a way where a user understands exactly what needs to be done

Comment on lines +211 to +213
return fmt.Errorf(
"pod name %q exceeds the maximum DNS label length of %d characters",
longestPodName, k8svalidation.DNS1123LabelMaxLength)
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Here the longestPodName might not be actually present in the cluster, so will be difficult for a user to understand where it's coming from.
Can we rephrase to convey something like the rackConfig/szie results in new pods names that will exceed the DNS label length

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.

2 participants