Catalog handling to filter plans/services in case of cross consumption #207

subhankarc · 2018-02-26T09:56:33Z

Parameterized the path that gets registered as broker.
Added settings for supported_platform and osbapi_compliant_name
Catalogs are shown based on the supported_platform
Changed implementation for PlatformManagers, instance assignment is removed from it.
Added new test for K8S catalog
Fixed Quota test which was changing the catalog
Removed the unnecessary CC API version check

Also, see cloudfoundry/servicebroker#442 and cloudfoundry/servicebroker#452

* Parameterized the path that gets registered as broker. * Added settings for supported_platform and osbapi_compliant_name * Catalogs are shown based on the supported_platform * Changed implementation for PlatformManagers, instance assignment is removed from it. * Added new test for K8S catalog * Fixed Quota test which was changing the catalog

cfdreddbot · 2018-02-26T09:56:34Z

Hey subhankarc!

Thanks for submitting this pull request!

All pull request submitters and commit authors must have a Contributor License Agreement (CLA) on-file with us. Please sign the appropriate CLA (individual or corporate).

When sending signed CLA please provide your github username in case of individual CLA or the list of github usernames that can make pull requests on behalf of your organization.

If you are confident that you're covered under a Corporate CLA, please make sure you've publicized your membership in the appropriate Github Org, per these instructions.

Once you've publicized your membership, one of the owners of this repository can close and reopen this pull request, and dreddbot will take another look.

cf-gitbot · 2018-02-26T09:56:34Z

We have created an issue in Pivotal Tracker to manage this:

https://www.pivotaltracker.com/story/show/155509320

The labels on this github issue will be updated when the story is started.

cloudfoundry/servicebroker#452

CC API version check was added to support 2.8 version. Removing that check now.

kamath-prasad

minor comments.

kamath-prasad · 2018-03-05T15:21:05Z

lib/fabrik/Fabrik.js

  }

+  static getPlatformManager(platform) {
+    const PlatformManager = (platform && CONST.PLATFORM_MANAGER[platform]) ? require(`./${CONST.PLATFORM_MANAGER[platform]}`) : undefined;


attribute name should start with small case here - platformManager

As it is a class reference, kept it as PlatformManager

kamath-prasad · 2018-03-05T15:34:05Z

config/settings.yml

      supportUrl: 'https://sap.com/'
    dashboard_client:
      id: 'service-fabrik-blueprint-client'
      secret: 'service-fabrik-blueprint-secret'


maybe its better to have a new plan to have support for k8s and leave the current one's for cf as the plan name in k8s has to be in different format ?!

There will not be any new plan. The same existing BOSH plans will be shown for K8S as well.

jagadish-kb

Per discussion please also update the method ensurePlatformContext.

jagadish-kb · 2018-03-07T03:47:14Z

lib/constants.js

+
  PLATFORM_MANAGER: {
-    'cloudfoundry': 'CfPlatformManager'
+    'cloudfoundry': 'CfPlatformManager',


instead of duping, would it rather make sense to set PLATFORM_ALIAS in context at the top of broker processing? that way we always access the manager class via the alias?

I have change this and removed the duped ones. Have handled that in the getPlatformManager code.

jagadish-kb · 2018-03-07T03:54:12Z

lib/controllers/ServiceBrokerApiController.js

          return;
        } else {
          throw new PreconditionFailed(`At least Broker API version ${minVersion} is required.`);
        }


The below ensures that if the calls from CF are coming in, then for the dependencies of SF on CF are checked via this. This might be true always, but nevertheless a check that could help in rarest of scenarios. Obviously if we are never going to increase the version of minCloudControllerApiVersion & we for sure know CF is already upgraded above this version then this check is unnecessary. Nevertheless worth a discussion before removal.

Post discussion, this is not needed. Keeping this comment for history.

jagadish-kb · 2018-03-07T04:15:06Z

lib/fabrik/BasePlatformManager.js


-  get platform() {
-    return this.context.platform;
+  getPlatformSpecificCatalog(catalog) {


Platformmanager is to return things platformSpecifc :-), so may be can we strip this from the method name and just keep it getCatalog. Also can return undefined from BasePlatformManager as its implementation from specific manager is absent.

:) changed it.

changing the name of method getCatalog in platform specific catalog filtering Removed the duped key values

jagadish-kb

good to go!

cf-gitbot added the unscheduled label Feb 26, 2018

subhankarc requested a review from jagadish-kb February 26, 2018 09:57

subhankarc added the DO NOT MERGE label Feb 26, 2018

subhankarc added 4 commits March 2, 2018 15:54

Merge branch 'master' into feature/crossconsumption

13f961e

Fix jshint errors

0cbc78d

Removing Plan name handling code as the osbapi change is LGTMed

51dadf9

cloudfoundry/servicebroker#452

Removing unnecessary CC api version check

f0eb52e

CC API version check was added to support 2.8 version. Removing that check now.

subhankarc changed the title ~~Catalog handling in case of cross consumption~~ Catalog handling to filter plans/services in case of cross consumption Mar 5, 2018

subhankarc requested review from kamath-prasad and muralisc March 5, 2018 04:34

kamath-prasad suggested changes Mar 5, 2018

View reviewed changes

kamath-prasad approved these changes Mar 6, 2018

View reviewed changes

jagadish-kb suggested changes Mar 7, 2018

View reviewed changes

Incorporating review comment.

cad8ee2

changing the name of method getCatalog in platform specific catalog filtering Removed the duped key values

jagadish-kb approved these changes Mar 7, 2018

View reviewed changes

jagadish-kb merged commit b9cc514 into cloudfoundry:master Mar 7, 2018

cf-gitbot removed the unscheduled label Mar 7, 2018

cf-gitbot added delivered accepted and removed delivered labels Apr 18, 2018

subhankarc deleted the feature/crossconsumption branch June 14, 2018 03:41

Catalog handling to filter plans/services in case of cross consumption #207

Catalog handling to filter plans/services in case of cross consumption #207

Uh oh!

Conversation

subhankarc commented Feb 26, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cfdreddbot commented Feb 26, 2018

Uh oh!

cf-gitbot commented Feb 26, 2018

Uh oh!

kamath-prasad left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagadish-kb left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jagadish-kb left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

subhankarc commented Feb 26, 2018 •

edited

Loading