Specify individual backupRepo availability in SolrCloud Status #358
Conversation
There was a problem hiding this comment.
This looks great!
There were a few things I noticed in the crd spec that seemed like they might've been oversights (esp inconsistency around case-sensitivity and min-length). But otherwise it looks "ready to go" to me.
It's got my 👍 as long as you lmk whether those crd spec things were intended or not.
| @@ -385,6 +385,8 @@ type SolrBackupRestoreOptions struct { | |||
| type SolrBackupRepository struct { | |||
| // A name used to identify this local storage profile. Values should follow RFC-1123. (See here for more details: | |||
| // https://kubernetes.io/docs/concepts/overview/working-with-objects/names/#dns-label-names) | |||
| // | |||
| // +kubebuilder:validation:Pattern:=[a-zA-Z0-9]([-_a-zA-Z0-9]*[a-zA-Z0-9])? | |||
There was a problem hiding this comment.
[-1/?] This regex allows capitalized letters, but the validation on the 'solrCloud' string in solrbackup_types.go looks like it requires all lowercase.
Was that an oversight, or is there some rationale for that that I'm missing?
There was a problem hiding this comment.
So this is just a name for the backup repository, it is pretty independent from the name of the SolrCloud (which has limitations given by Kubernetes, which I linked below).
I used the same regex, but changed it to allow capital letters and underscores, since we don't need to be as strict with the names of the repositories (as far as I know).
Is there any limitation in Solr for the name of backup repositories? If so we should definitely change this to match.
| type: string | ||
| solrCloud: | ||
| description: A reference to the SolrCloud to create a backup for | ||
| minLength: 63 | ||
| pattern: '[a-z0-9]([-a-z0-9]*[a-z0-9])?' |
There was a problem hiding this comment.
[-1/Q] Ditto to my reminder comment above, but this time regarding the lower-case requirement on this string field that you may or may not have intended.
There was a problem hiding this comment.
So yeah, just to be very clear, I'm just following the guidelines for kubernetes resource names. This field is a reference to SolrCloud resources, so there is no way that giving a string that has a length > 63, or doesn't match that regex, is going to be successful (because we will never be able to find a SolrCloud in kubernetes with the given name).
|
Will test (and fix if need be) and merge tomorrow. |
Resolves #326
This uses a map of the BackupRepositories names in the SolrCloud status, mapping the name of the repo to a boolean of whether that repo is available across all nodes.
TODO: