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

Service Binding From Add Flow #3958

Merged
merged 1 commit into from Jan 25, 2020
Merged

Service Binding From Add Flow #3958

merged 1 commit into from Jan 25, 2020

Conversation

andrewballantyne
Copy link
Contributor

@andrewballantyne andrewballantyne commented Jan 14, 2020 •
edited

Resolves https://issues.redhat.com/browse/ODC-2601

TODO:

  • Git Import
  • Deploy Image

We'll address these issues in a follow-up ticket. We should just get the code in for now to build that foundation.

  • Test service binding
  • Tests
  • Catalog deployment

Given an application / workload setup like this:
Screen Shot 2020-01-14 at 12 38 05 PM

Using the work to redirect to the add flow being done in #3918 to populate the URL with an application + contextSource:

InContextAddConnector

The slowness of the connector being added can be attributed to this issue: https://issues.redhat.com/browse/ODC-2726

cc @openshift/team-devconsole-ux

For testing purposes while #3918 is being reviewed... Here are the steps to reproduce the above scenario:

  1. Create a new project
  2. Add a resource "From Git" (using https://github.com/nodeshift-starters/react-web-app)
  3. Accept all the defaults & save the form
  4. Use this URL (your-namespace will need to be updated to your chosen project in no.1) http://localhost:9000/import/ns/your-namespace?importType=git&application=react-web-app-app&contextSource=core~v1~Deployment%2Freact-web-app
    (using https://github.com/nodeshift-starters/nodejs-rest-http)

Key parts here are the application=react-web-app-app and contextSource=core~v1~Deployment%2Freact-web-app (ie groupVersionKind/app-name)

@openshift-ci-robot openshift-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 14, 2020
@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 14, 2020
andrewballantyne
andrewballantyne commented Jan 14, 2020
View reviewed changes
frontend/packages/dev-console/src/components/import/ImportForm.tsx
.then((resources) =>
doContextualBinding(resources, contextualSource, serviceBindingAvailable),
)
.catch(() => {});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is related to the bug in the description (ODC-2726); it catches and swallows the errors 😞

frontend/packages/dev-console/src/components/topology/topology-utils.ts
@@ -321,18 +322,8 @@ export const transformTopologyData = (
filters?: TopologyFilters,
): TopologyDataModel => {
const installedOperators = _.get(resources, 'clusterServiceVersions.data');
let operatorBackedServiceKindMap: OperatorBackedServiceKindMap;
const operatorBackedServiceKindMap = getOperatorBackedServiceKindMap(installedOperators);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactor this to reuse it. From what I can tell no one checked for undefined, so an empty map is always returned for the purity of getOperatorBackedServiceKindMap.

frontend/packages/dev-console/src/const.ts
Comment on lines +5 to +11
/** URL query params that adjust scope / purpose of the page */
export enum QUERY_PROPERTIES {
/** For defining a contextual application group (ie, add new workload into this application group) */
APPLICATION = 'application',
/** For defining a contextual source of the redirect (ie, connect a new workload from this contextual source) */
CONTEXT_SOURCE = 'contextSource',
}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hope for reuse here... not sure if that's logical or not. But at least @karthikjeeyar you'll need to make use of this in your PR #3918 (probably after your PR is merged we clean it up)

@andrewballantyne
Copy link
Contributor Author

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 14, 2020
@nemesis09 nemesis09 mentioned this pull request Jan 20, 2020
Merged
@andrewballantyne andrewballantyne changed the title [WIP] Service Binding From Add Flow Service Binding From Add Flow Jan 23, 2020
@openshift-ci-robot openshift-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jan 23, 2020
@invincibleJai
Copy link
Member

invincibleJai commented Jan 24, 2020 •
edited

Verified locally as well, overall looks good :)

good to have specs for utils, but as mentioned in description it can go as a follow-up PR

@andrewballantyne
Copy link
Contributor Author

Verified locally as well, overall looks good :)

good to have specs for utils, but as mentioned in description it can go as a follow-up PR

I added https://issues.redhat.com/browse/ODC-2824 to make sure the tests get done next sprint.

@invincibleJai
Copy link
Member

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-bot
Copy link
Contributor

/retest

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

3 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.

@openshift-bot
Copy link
Contributor

/retest

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

@andrewballantyne
Copy link
Contributor Author

/retest

@openshift-bot
Copy link
Contributor

/retest

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

@spadgett
Copy link
Member

/hold
for merge queue fix #4065

@openshift-ci-robot openshift-ci-robot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 24, 2020
christianvogt
christianvogt reviewed Jan 24, 2020
View reviewed changes
frontend/packages/dev-console/src/components/QueryFocusApplication.tsx Outdated
searchParams,
onSetApp,
}) => {
const [originalApp] = React.useState(application);
Copy link
Contributor

Choose a reason for hiding this comment

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

useRef ?

frontend/packages/dev-console/src/components/QueryFocusApplication.tsx Outdated
onSetApp,
}) => {
const [originalApp] = React.useState(application);
const desiredApplication = searchParams.get(QUERY_PROPERTIES.APPLICATION);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe avoid the searchParams prop altogether and do this?

Suggested change
const desiredApplication = searchParams.get(QUERY_PROPERTIES.APPLICATION);
const desiredApplication = new URLSearchParams(location.search).get(QUERY_PROPERTIES.APPLICATION);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would need to be window.location.search for lint... but sure.

frontend/packages/dev-console/src/utils/application-utils.ts Outdated
const operatorBackedServiceKindMap = getOperatorBackedServiceKindMap(
await k8sList(ClusterServiceVersionModel, { ns: namespace }),
);
const ownerResourceKind = _.get(newResource, 'metadata.ownerReferences[0].kind');
Copy link
Contributor

Choose a reason for hiding this comment

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

Should move to optional chaining.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah right, wasn't merged when I did this.

frontend/packages/dev-console/src/utils/application-utils.ts Outdated
let operatorBackedServiceKindMap: OperatorBackedServiceKindMap = {};
if (installedOperators) {
operatorBackedServiceKindMap = installedOperators.reduce((kindMap, csv) => {
_.get(csv, 'spec.customresourcedefinitions.owned', []).forEach((crd) => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should move to optional chaining.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@christianvogt
Copy link
Contributor

/lgtm

@openshift-ci-robot openshift-ci-robot added the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
@openshift-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, christianvogt, invincibleJai

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@spadgett
Copy link
Member

/hold cancel
/retest

@openshift-ci-robot openshift-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Jan 25, 2020
openshift-merge-robot added a commit that referenced this pull request Jan 25, 2020
@openshift-merge-robot
Merge pull request #3958 from andrewballantyne/in-context-application
293553e 
Service Binding From Add Flow
@openshift-merge-robot openshift-merge-robot merged commit 293553e into openshift:master Jan 25, 2020
@openshift-merge-robot openshift-merge-robot merged commit 38f70e6 into openshift:master Jan 25, 2020
@spadgett spadgett added this to the v4.4 milestone Jan 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@invincibleJai invincibleJai invincibleJai left review comments

@christianvogt christianvogt christianvogt left review comments

@debsmita1 debsmita1 Awaiting requested review from debsmita1

@karthikjeeyar karthikjeeyar Awaiting requested review from karthikjeeyar

Assignees

@invincibleJai invincibleJai

@christianvogt christianvogt

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. component/dev-console Related to dev-console kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
None yet
Milestone
v4.4
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@andrewballantyne @invincibleJai @openshift-bot @spadgett @christianvogt @openshift-ci-robot @openshift-merge-robot