Skip to content
This repository has been archived by the owner on May 6, 2022. It is now read-only.

Fix common validations regression #1882

Merged

Conversation

eriknelson
Copy link
Contributor

@eriknelson eriknelson commented Mar 28, 2018

Discovered a regression that snuck in with my common validation refactoring. ClusterT specific validation errors were getting thrown away due to a bad commonErrs append. This correctly appends to the original allErrs slice.

@k8s-ci-robot k8s-ci-robot added size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Mar 28, 2018
@jpeeler jpeeler added the LGTM1 label Mar 28, 2018
Copy link
Contributor

@carolynvs carolynvs left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -90,7 +90,7 @@ func validateClusterServiceBrokerSpec(spec *sc.ClusterServiceBrokerSpec, fldPath
commonErrs := validateCommonServiceBrokerSpec(&spec.CommonServiceBrokerSpec, fldPath)

if len(commonErrs) != 0 {
allErrs = append(commonErrs)
allErrs = append(allErrs, commonErrs...)
Copy link
Contributor

Choose a reason for hiding this comment

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

Doh! I'm saddened that go didn't warn you about that! Silly go...

Copy link
Contributor Author

@eriknelson eriknelson Mar 28, 2018

Choose a reason for hiding this comment

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

I was surprised as well; wondering when that behavior would even be desired? Tricky...

Copy link
Contributor

Choose a reason for hiding this comment

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

I am guessing it's the equivalent of this

allErrs = append(allErrs, []error{}...)

@carolynvs carolynvs merged commit c0035bc into kubernetes-retired:master Mar 28, 2018
@eriknelson eriknelson deleted the common-validations-fix branch March 28, 2018 19:42
orthros added a commit to orthros/service-catalog that referenced this pull request Apr 4, 2018
* master:
  Support provisioning by external id (kubernetes-retired#1756)
  Add namespaced ServiceBroker API (kubernetes-retired#1881)
  Enable verbose logging in svcat (kubernetes-retired#1822)
  Adding ServicePlan (namespaced plans) API (kubernetes-retired#1894)
  Svcat bind params now supports --params-json (kubernetes-retired#1889)
  words discussing the automated builds (kubernetes-retired#1885)
  v0.1.12 release changes
  don't overwrite generated files before verifying (kubernetes-retired#1891)
  Update registry code from broker to clusterservicebroker (kubernetes-retired#1880)
  Credentials remapping (kubernetes-retired#1868)
  Rename/move existing "ServicePlan" things (kubernetes-retired#1883)
  Start orphan mitigation after receiving a  last operation status for async provisioning (kubernetes-retired#1886)
  Update of name in example ServiceClass (kubernetes-retired#1878)
  Cluster-id configmap (kubernetes-retired#1658)
  Add namespaced ServiceClass API (kubernetes-retired#1859)
  Fix common validations regression (kubernetes-retired#1882)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. LGTM1 LGTM2 size/XS Denotes a PR that changes 0-9 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants