Bug 1686668: [csc] Ignore invalid values when reading csc.Spec.Packages
#138
Conversation
openshift-ci-robot
added
the
size/S
| @@ -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.
You should also fix getCommand() and have all that string fixup happen in just one place.
|
|
||
| pkgSlice := strings.Split(strings.Replace(pkgs, " ", "", -1), ",") | ||
|
|
||
| for _, pkg := range pkgSlice { |
There was a problem hiding this comment.
I would recommend trimming white spaces from each package name. After trimming, if the package name is not empty we add it.
| func getValidPackageSliceFromString(pkgs string) []string { | ||
| pkgIds := make([]string, 0) | ||
|
|
||
| pkgSlice := strings.Split(strings.Replace(pkgs, " ", "", -1), ",") |
There was a problem hiding this comment.
Is strings.Replace required If we sanitize each package name by trimming white spaces? For a leading comma strings.Split returns a package name with a space.
There was a problem hiding this comment.
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.
Additionally I prefer this approach because removing all of the whitespace is needed anyway. This also avoids the issue of spaces in package names.
reading `csc.Spec.Packages` Problem: In some cases, when the Openshift UI updates the package list of a csc after removing a subscription, the update does not properly format the package list. For example: `packages: ',strimzi-kafka-operator-test'` In this example, the package list accidentally includes a leading comma. Right now, the marketplace doesn't handle poorly formatted package lists in the csc spec. In general, if a user accidentally leaves a comma behind the operator should ignore it and proceed as normal with the list of valid packages. Solution: Any time the operator reads the package list, that read happens from a public method that formats the package list and excludes invalid packages (specifically empty string). Added a private method `getValidPackageSliceFromString()` that takes a package string and removes invalid elements from it. Updated the two `GetPackages()` methods on the csc and spec types to call this method before returning. Added a new method on the csc type called `GetPackages()` that uses the validation logic to reconstruct the `Spec.Packages` string. Updated every case where we were referencing the `Spec.Packages` field directly and replaced it with one of these methods. Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1686668
|
@tkashem @aravindhp @awgreene Please take a look when you get a chance. Thanks. |
openshift-ci-robot
added
the
approved
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: aravindhp, awgreene, kevinrizza The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
Problem:
In some cases, when the Openshift UI updates the package list of a csc
after removing a subscription, the update does not properly format the
package list. For example:
packages: ',strimzi-kafka-operator-test'In this example, the package list accidentally includes a leading comma.
Right now, the marketplace doesn't handle poorly formatted package
lists in the csc spec. In general, if a user accidentally leaves a comma
behind the operator should ignore it and proceed as normal with the
list of valid packages.
Solution:
Any time the operator reads the package list, that read happens
from a public method that formats the package list and excludes invalid
packages (specifically empty string).
Added a private method
getValidPackageSliceFromString()that takes apackage string and removes invalid elements from it.
Updated the two
GetPackages()methods on the csc and spec types tocall this method before returning.
Added a new method on the csc type called
GetPackages()that uses thevalidation logic to reconstruct the
Spec.Packagesstring.Updated every case where we were referencing the
Spec.Packagesfielddirectly and replaced it with one of these methods.