You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@solr.apache.org by GitBox <gi...@apache.org> on 2021/11/01 15:18:14 UTC
[GitHub] [solr-operator] gerlowskija commented on a change in pull request #358: Specify individual backupRepo availability in SolrCloud Status
gerlowskija commented on a change in pull request #358:
URL: https://github.com/apache/solr-operator/pull/358#discussion_r740274993
##########
File path: api/v1beta1/solrcloud_types.go
##########
@@ -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])?
Review 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?
##########
File path: helm/solr-operator/crds/crds.yaml
##########
@@ -1053,9 +1053,12 @@ spec:
type: object
repositoryName:
description: The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the auto-configured repository for legacy singleton volumes).
+ pattern: '[a-zA-Z0-9]([-_a-zA-Z0-9]*[a-zA-Z0-9])?'
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])?'
Review 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.
##########
File path: helm/solr-operator/crds/crds.yaml
##########
@@ -1053,9 +1053,12 @@ spec:
type: object
repositoryName:
description: The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the auto-configured repository for legacy singleton volumes).
+ pattern: '[a-zA-Z0-9]([-_a-zA-Z0-9]*[a-zA-Z0-9])?'
type: string
solrCloud:
description: A reference to the SolrCloud to create a backup for
+ minLength: 63
Review comment:
[-1/Q] I commented above on this value in `solr.apache.org_solrbackups.yaml`, suspecting it was something you'd set for testing but hadn't removed yet. If that ends up being the case, I figured I'd comment here as a reminder to make sure that your change gets propagated to this file as well.
##########
File path: config/crd/bases/solr.apache.org_solrbackups.yaml
##########
@@ -1053,9 +1053,12 @@ spec:
type: object
repositoryName:
description: The name of the repository to use for the backup. Defaults to "legacy_local_repository" if not specified (the auto-configured repository for legacy singleton volumes).
+ pattern: '[a-zA-Z0-9]([-_a-zA-Z0-9]*[a-zA-Z0-9])?'
type: string
solrCloud:
description: A reference to the SolrCloud to create a backup for
+ minLength: 63
Review comment:
[-1] I imagine you put this in for testing at some point or something?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@solr.apache.org
For additional commands, e-mail: issues-help@solr.apache.org