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
Bug 1686668: [csc] Ignore invalid values when reading csc.Spec.Packages
#138
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -100,9 +100,14 @@ func (csc *CatalogSourceConfig) EnsurePublisher() { | |
} | ||
} | ||
|
||
func (csc *CatalogSourceConfig) GetPackages() string { | ||
pkgs := getValidPackageSliceFromString(csc.Spec.Packages) | ||
return strings.Join(pkgs, ",") | ||
} | ||
|
||
// GetPackageIDs returns the list of package(s) specified. | ||
func (csc *CatalogSourceConfig) GetPackageIDs() []string { | ||
return strings.Split(strings.Replace(csc.Spec.Packages, " ", "", -1), ",") | ||
return getValidPackageSliceFromString(csc.Spec.Packages) | ||
} | ||
|
||
// GetTargetNamespace returns the TargetNamespace where the OLM resources will | ||
|
@@ -129,5 +134,19 @@ func (csc *CatalogSourceConfig) RemoveOwner(ownerUID types.UID) { | |
|
||
// GetPackageIDs returns the list of package(s) specified. | ||
func (spec *CatalogSourceConfigSpec) GetPackageIDs() []string { | ||
return strings.Split(strings.Replace(spec.Packages, " ", "", -1), ",") | ||
return getValidPackageSliceFromString(spec.Packages) | ||
} | ||
|
||
func getValidPackageSliceFromString(pkgs string) []string { | ||
pkgIds := make([]string, 0) | ||
|
||
pkgSlice := strings.Split(strings.Replace(pkgs, " ", "", -1), ",") | ||
|
||
for _, pkg := range pkgSlice { | ||
aravindhp marked this conversation as resolved.
Show resolved
Hide resolved
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I would recommend trimming white spaces from each package name. After trimming, if the package name is not empty we add it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. See below. |
||
if pkg != "" { | ||
pkgIds = append(pkgIds, pkg) | ||
} | ||
} | ||
|
||
return pkgIds | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -96,7 +96,7 @@ func (r *registry) GetAddress() string { | |
// ensureDeployment ensures that registry Deployment is present for serving | ||
// the the grpc interface for the packages from the given operatorSources | ||
func (r *registry) ensureDeployment(operatorSources string) error { | ||
registryCommand := getCommand(r.csc.Spec.Packages, operatorSources) | ||
registryCommand := getCommand(r.csc.GetPackages(), operatorSources) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should also fix There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Addressed. |
||
deployment := new(DeploymentBuilder).WithTypeMeta().Deployment() | ||
if err := r.client.Get(context.TODO(), r.csc.key(), deployment); err != nil { | ||
deployment = r.newDeployment(registryCommand) | ||
|
@@ -402,7 +402,7 @@ func (r *registry) waitForDeploymentScaleDown(retryInterval, timeout time.Durati | |
|
||
// getCommand returns the command used to launch the registry server | ||
func getCommand(packages string, sources string) []string { | ||
return []string{"appregistry-server", "-s", sources, "-o", strings.Replace(packages, " ", "", -1)} | ||
return []string{"appregistry-server", "-s", sources, "-o", packages} | ||
} | ||
|
||
// getRules returns the PolicyRule needed to access the given operatorSources and secrets | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is
strings.Replace
required If we sanitize each package name by trimming white spaces? For a leading commastrings.Split
returns a package name with a space.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do it to the initial string once that will remove any extra whitespace, meaning we don't need to call
strings.Replace()
on each token.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Additionally I prefer this approach because removing all of the whitespace is needed anyway. This also avoids the issue of spaces in package names.