Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Rebase service catalog to v0.0.17 #16150

Merged
merged 3 commits into from
Sep 13, 2017
Merged

Rebase service catalog to v0.0.17 #16150

merged 3 commits into from
Sep 13, 2017

Conversation

jpeeler
Copy link

@jpeeler jpeeler commented Sep 5, 2017

This rebases service catalog to the latest release.

@openshift-ci-robot openshift-ci-robot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label Sep 5, 2017
@pmorie
Copy link
Contributor

pmorie commented Sep 5, 2017

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 5, 2017
@openshift-merge-robot openshift-merge-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 5, 2017
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jpeeler
Copy link
Author

jpeeler commented Sep 5, 2017

/test extended_conformance_gce

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@pmorie
Copy link
Contributor

pmorie commented Sep 6, 2017

@jpeeler can you determine whether this version has the same issue as #16141

@pmorie
Copy link
Contributor

pmorie commented Sep 6, 2017

/lgtm cancel

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Sep 6, 2017
@openshift-merge-robot openshift-merge-robot removed the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 6, 2017
@pmorie pmorie mentioned this pull request Sep 7, 2017
Closed
@pmorie
Copy link
Contributor

pmorie commented Sep 7, 2017

@jpeeler we need to ensure that:

  • The templates in openshift are updated for the new resource names
  • Any new admission controllers in the upstream are present in the openshift templates

We'll also need a follow-up after this to update the ansible installer.

@jmontleon
Copy link
Contributor

jmontleon commented Sep 8, 2017 •
edited
Loading

I built images with this PR in attempt to use the updated service catalog but I am seeing an error from oc cluster up --service-catalog=true

-- Installing template service broker ... FAIL
   Error: failed to register broker with service catalog: timed out waiting for the condition

Edit: I had to use a newer oc. Now I am getting:

   Error: cannot register the template service broker
   Caused By:
     Error: cannot create objects from template openshift-infra/template-service-broker-registration
     Caused By:
       Error: unable to recognize servicecatalog.k8s.io/v1alpha1, Kind=Broker: no matches for servicecatalog.k8s.io/, Kind=Broker

which looks similar to our Broker before we updated the kind. I think I know where to fix this. I'll add a note if my test works.

@jpeeler
Copy link
Author

jpeeler commented Sep 8, 2017

@jmontleon This PR was canceled for merging because it's not ready to go after all. One problem that needs fixing is the kind in the template-service-broker-registration needs to be changed to ServiceBroker. There may be additional renames required as well (since broker is not the only name change that occurred in 0.0.17).

@spadgett spadgett mentioned this pull request Sep 11, 2017
Merged
@jmontleon
Copy link
Contributor

@jpeeler @spadgett I had some luck getting APB's to at least launch with additional changes.

origin-web-common: https://github.com/jmontleon/origin-web-common/commit/33a85c42b13402f53ebd458066f306665368afaa
origin-web-catalog: https://github.com/jmontleon/origin-web-catalog/commit/63bf7b3fc9b06b0748e3b74470da7b10585c9593
origin-web-console: https://github.com/jmontleon/origin-web-console/commit/dfe895e0fe7796975c7a6bf86d426ee23871d258

With all that I updated the assets in origin, modified https://github.com/openshift/origin/blob/master/install/service-catalog-broker-resources/template-service-broker-registration.yaml#L13 to read ServiceBroker, and ran make update.

I also had to update the service catalog to newer than what's currently in the origin repo, but I have not had much luck following the sync documentation to share my overall changes just yet.

None of this addresses binds.

@spadgett
Copy link
Member

@jmontleon My PR should address binding, but I need to test:

openshift/origin-web-console#2048

Jeff Peeler added 3 commits September 11, 2017 13:43
Squashed 'cmd/service-catalog/go/src/github.com/kubernetes-incubator/…
2b7c9ef 
…service-catalog/' changes from 7e650e7e39..ef63307bdb

ef63307bdb origin build: add origin tooling
a876fe3 v0.0.17 (openshift#1178)
c5237fe correct osbapi service definition (openshift#1177)
6036d4e Adding walkthrough instructions for 1.7 (openshift#1171)
5f111dd Specifying that you need Helm v2.5.0 for installation (openshift#1170)
08043bd Adding more small fixes to the walkthrough & install docs (openshift#1169)
d65d4a1 rbac targets needed to be renamed as well (openshift#1161)
590f6f2 Write helm command to file for api aggregation (openshift#1141)
49ddcf6 clean before building a specific arch (openshift#1168)
43f7cfb Splitting up the Walkthrough for 1.6 and 1.7 instructions (openshift#1163)
02e0217 Updates to README (openshift#1166)
57f2aa5 Adding instructions for installing from Macs (openshift#1164)
dfe620e fix rate-limiting for polling queue (openshift#1143)
ca5f335 Use Generation instead of checksum for Broker (openshift#1145)
5364daa Merge branch 'pr/1158'
f34c5db move Travis deployment script to directory in 'contrib/'
2a00d7f Update incorrect port (openshift#1156)
b0ed60e improve the repository's layout (openshift#1154)
f870baf Follow up file / renames from openshift#1142 (openshift#1152)
826b4f9 remove unnecessary json annotations (openshift#1153)
33cb345 Rename resources. closes openshift#1080 (openshift#1142)
70c2b9b Add ability to specify CA certs to use for TLS authentication. (openshift#1112)
2aa5039 v0.0.16 (openshift#1140)
65de49c Comments for unit test bullet proofing (openshift#1139)
REVERT: 7e650e7e39 origin build: add origin tooling

git-subtree-dir: cmd/service-catalog/go/src/github.com/kubernetes-incubator/service-catalog
git-subtree-split: ef63307bdbaa64efca204912f5361a4f3d3be2c8
Merge version v0.0.17 of Service Catalog from https://github.com/open…
dc92bcf 
…shift/service-catalog:v0.0.17+origin
catalog: rename resources and add admission controller
a94bc00
@spadgett spadgett mentioned this pull request Sep 12, 2017
Merged
@pmorie
Copy link
Contributor

pmorie commented Sep 12, 2017

@spadgett has told me in person that this PR worked well for him.

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Sep 12, 2017
@smarterclayton
Copy link
Contributor

/approve

@openshift-merge-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jpeeler, pmorie, smarterclayton

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

@openshift-merge-robot openshift-merge-robot added approved Indicates a PR has been approved by an approver from all required OWNERS files. needs-api-review labels Sep 12, 2017
@pmorie
Copy link
Contributor

pmorie commented Sep 12, 2017

/retest

@jmontleon
Copy link
Contributor

@jpeeler @pmorie any insight into why we're still getting errors when the controller-manager tries to communicate with the ansible service broker over SSL?

ERROR: logging before flag.Parse: W0912 16:46:12.130863      37 controller_broker.go:208] Error getting broker catalog for broker "ansible-service-broker": Get https://asb-1338-ansible-service-broker.172.18.0.1.nip.io/v2/catalog: x509: certificate signed by unknown authority

@jmontleon
Copy link
Contributor

jmontleon commented Sep 12, 2017 •
edited
Loading

Even with SSL disabled I also see errors that are preventing APB's and other apps from being properly populated in the UI.

ERROR: logging before flag.Parse: E0912 16:51:46.426900      64 reflector.go:201] github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61: Failed to list *v1alpha1.ServiceInstanceCredential: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list serviceinstancecredentials.servicecatalog.k8s.io at the cluster scope: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list all serviceinstancecredentials.servicecatalog.k8s.io in the cluster (get serviceinstancecredentials.servicecatalog.k8s.io)
ERROR: logging before flag.Parse: I0912 16:51:47.425899      64 reflector.go:236] Listing and watching *v1alpha1.ServiceBroker from github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61
ERROR: logging before flag.Parse: I0912 16:51:47.426863      64 reflector.go:236] Listing and watching *v1alpha1.ServiceInstance from github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61
ERROR: logging before flag.Parse: I0912 16:51:47.427976      64 reflector.go:236] Listing and watching *v1alpha1.ServiceInstanceCredential from github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61
ERROR: logging before flag.Parse: E0912 16:51:47.428047      64 reflector.go:201] github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61: Failed to list *v1alpha1.ServiceBroker: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list servicebrokers.servicecatalog.k8s.io at the cluster scope: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list all servicebrokers.servicecatalog.k8s.io in the cluster (get servicebrokers.servicecatalog.k8s.io)
ERROR: logging before flag.Parse: E0912 16:51:47.428410      64 reflector.go:201] github.com/kubernetes-incubator/service-catalog/pkg/client/informers_generated/externalversions/factory.go:61: Failed to list *v1alpha1.ServiceInstance: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list serviceinstances.servicecatalog.k8s.io at the cluster scope: User "system:serviceaccount:kube-service-catalog:service-catalog-controller" cannot list all serviceinstances.servicecatalog.k8s.io in the cluster (get serviceinstances.servicecatalog.k8s.io)

Edit: disregard this. It is working for me if I disabled SSL on the broker.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

2 similar comments
@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@openshift-bot
Copy link
Contributor

/retest

Please review the full test history for this PR and help us cut down flakes.

@jpeeler
Copy link
Author

jpeeler commented Sep 13, 2017

/retest

@jpeeler
Copy link
Author

jpeeler commented Sep 13, 2017

flake #16312

@pmorie
Copy link
Contributor

pmorie commented Sep 13, 2017

/retest

@openshift-merge-robot
Copy link
Contributor

Automatic merge from submit-queue (batch tested with PRs 16150, 16284, 16296, 16071)

@openshift-merge-robot openshift-merge-robot merged commit b9840a6 into openshift:master Sep 13, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@pmorie pmorie

@smarterclayton smarterclayton

@aveshagarwal aveshagarwal

Labels
api-approved approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. needs-api-review size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
9 participants
@jpeeler @pmorie @openshift-bot @jmontleon @spadgett @smarterclayton @openshift-merge-robot @aveshagarwal @openshift-ci-robot