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.

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

Add serviceClass url param to jump into service-catalog flow #2047

Conversation

david-martin
Copy link
Member

@david-martin david-martin commented Sep 11, 2017

https://trello.com/c/w8LiRRSg

If a serviceClass param is present, look up the serviceClass and
launch the service-catalog modal for that class.
The param can be the service id e.g. 'cakephp-mysql-persistent'
or the service display name e.g. CakePHP + MySQL (Persistent)

Fixes openshift/origin-web-catalog#423

The reason for allowing the service id or the service display is to allow for 'not very deterministic' id's.
This is somewhat the case for service classes returned by the ansible-service-broker.
For example, an apb for keycloak would have an generated serviceclass name of dh-myorg-keycloak-latest,
where dh is the image stream, myorg is the org, keycloak is the apb name and latest is the apb image tag. Having to know all 4 of these details seems a little contrived, rather than having a simple id.

@david-martin
Copy link
Member Author

@spadgett This is my first attempt at openshift/origin-web-catalog#423.
I'm interested in your thoughts on the param value and what field to lookup the class on (see description)

@david-martin david-martin force-pushed the url-params-to-jump-into-service-catalog-flow branch 2 times, most recently from 8cd308f to 62420aa Compare September 11, 2017 16:08
Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

Hey @david-martin, thank you for the contribution.

I'm really hesitant to search display names because they're not unique and can change. Whereas the kubernetes name has to be unique and immutable. It would be easy for a broker to accidentally (or on purpose) change what service a link provisions by changing the display name of one its services.

var paramItem = _.find($scope.catalogItems, {
name: paramClass
});
// Lookup the class by display name e.g. "CakePHP + MySQL (Persistent)"
Copy link
Member

Choose a reason for hiding this comment

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

It looks like you might have the order backwards here, checking display name first above and then metadata.name here.

@david-martin
Copy link
Member Author

@spadgett Understood. I'm in agreement after thinking through it some more. I'll modify to search the kubernetes name only.

@david-martin david-martin force-pushed the url-params-to-jump-into-service-catalog-flow branch from 62420aa to 6b21f70 Compare September 11, 2017 17:02
If a `serviceClass` param is present, look up the serviceClass and
launch the service-catalog modal for that class.
@david-martin david-martin force-pushed the url-params-to-jump-into-service-catalog-flow branch from 6b21f70 to 2f75b67 Compare September 11, 2017 17:03
@david-martin
Copy link
Member Author

@spadgett Anything else I can do here? Is it enough to satisfy the card? https://trello.com/c/w8LiRRSg/1050-url-params-to-jump-into-service-catalog-flow
I'm not sure what exactly this part means in the context of the web console "So that I can see an unauthenticated view of applications"

Copy link
Member

@spadgett spadgett left a comment

Choose a reason for hiding this comment

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

LGTM, but will need to wait until Monday when master reopens for features

@spadgett
Copy link
Member

Anything else I can do here? Is it enough to satisfy the card?

It should satisfy the card.

@spadgett
Copy link
Member

[merge]

@openshift-bot
Copy link

Evaluated for origin web console merge up to 2f75b67

@openshift-bot
Copy link

openshift-bot commented Sep 18, 2017

Origin Web Console Merge Results: SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_request_origin_web_console/206/) (Base Commit: 1b695d8) (PR Branch Commit: 2f75b67)

@openshift-bot openshift-bot merged commit 0ce60c1 into openshift:master Sep 18, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants