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

Bug 1806914: redirect to topology when resource created from dev-catalog #4459

Merged

Conversation

sahil143
Copy link
Contributor

ODC-Bug: https://issues.redhat.com/browse/ODC-3152

This PR adds functionality to redirect page to the topology page when a resource is installed from dev-catalog.

Template
t-to

Operator-backed
ob-to

@openshift-ci-robot
Copy link
Contributor

@sahil143: This pull request references Bugzilla bug 1806914, which is valid. The bug has been moved to the POST state. The bug has been updated to refer to the pull request using the external bug tracker.

In response to this:

Bug 1806914: redirect to topology when resource created from dev-catalog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-ci-robot openshift-ci-robot added bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 25, 2020
@openshift-ci-robot openshift-ci-robot added component/core Related to console core functionality component/olm Related to OLM labels Feb 25, 2020
@sahil143 sahil143 changed the title Bug 1806914: redirect to topology when resource created from dev-catalog [WIP] Bug 1806914: redirect to topology when resource created from dev-catalog Feb 25, 2020
@openshift-ci-robot openshift-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Feb 25, 2020
)}/${referenceForModel(operandModel)}`,
),
)
.then(() => history.push(`/topology`))
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe check for dev perspective? Not sure we want to do this at all times.

@andrewballantyne
Copy link
Contributor

/kind bug

@openshift-ci-robot openshift-ci-robot added the kind/bug Categorizes issue or PR as related to a bug. label Feb 25, 2020
@sahil143 sahil143 changed the title [WIP] Bug 1806914: redirect to topology when resource created from dev-catalog Bug 1806914: redirect to topology when resource created from dev-catalog Feb 26, 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 Feb 26, 2020
@sahil143
Copy link
Contributor Author

/retest

)}/${referenceForModel(operandModel)}`,
),
)
params.has('redirect')
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not use perspective extension to get the redirect URL based on perspective similar to how import from git does it right now?

@@ -346,7 +357,7 @@ export class CatalogTileViewPage extends React.Component<
<div className="co-catalog-page__overlay-actions">
<Link
className="pf-c-button pf-m-primary co-catalog-page__overlay-action"
to={detailsItem.href}
to={this.appendTopologyRedirect(detailsItem.href)}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's just hard coding the URL redirect to topology which means that user can be in any perspective and still he will be redirected to topology. Is this the behavior that we want here? I would prefer if the redirect happens based on current perspective. cc: @christianvogt

Below screencast shows that even if the user is in admin perspective, he is redirected to topology.
Peek 2020-02-29 01-56

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 can be avoided now that the implementation is changed to use active Perspective.

)}/${props.match.params.plural}`;
const resourceObjPath = () => {
const params = new URLSearchParams(window.location.href);
return params.has('redirect')
Copy link
Contributor

Choose a reason for hiding this comment

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

This doesn't work for operator backed services when I tested it. For some reason the redirect param that is added is being removed on this page which results in user getting redirected to default admin URL.

Peek 2020-02-29 01-54

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@rohitkrai03 Not sure why the param is being removed but I changed the implementation to use activePerspective.

@rohitkrai03
Copy link
Contributor

/assign

Copy link
Contributor

@rohitkrai03 rohitkrai03 left a comment

Choose a reason for hiding this comment

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

/lgtm

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

FYI @alecmerdler @spadgett

We want to provide a catalog extension in the future for contributing items to the dev catalog. In doing that we will refactor how operands are created via the catalog to avoid reusing the same shared route. We will then remove the additions here which redirect to topology.

@spadgett
Copy link
Member

spadgett commented Mar 2, 2020

What happens when the template instance fails?

This might also be really confusing if the template doesn't create any resources that are visible in the topology.

@spadgett
Copy link
Member

spadgett commented Mar 2, 2020

I have a similar question for operator-backed resources. Will the thing you create and its status always be apparent from the topology page?

@christianvogt
Copy link
Contributor

christianvogt commented Mar 2, 2020

@spadgett had the same concerns before and UX said they wanted to always redirect to topology out of the dev catalog in the developer perspective.

cc @openshift/team-devconsole-ux

@christianvogt
Copy link
Contributor

/approve

@openshift-ci-robot openshift-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Mar 3, 2020
@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Mar 4, 2020
@andrewballantyne
Copy link
Contributor

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@andrewballantyne
Copy link
Contributor

/test e2e-gcp-console

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

@openshift-ci-robot
Copy link
Contributor

openshift-ci-robot commented Mar 5, 2020

@sahil143: The following test failed, say /retest to rerun all failed tests:

Test name Commit Details Rerun command
ci/prow/verify f079ca5 link /test verify

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@openshift-bot
Copy link
Contributor

/retest

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

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

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

@sahil143
Copy link
Contributor Author

sahil143 commented Mar 5, 2020

/retest

@sahil143
Copy link
Contributor Author

sahil143 commented Mar 5, 2020

/cherrypick release-4.4

@openshift-cherrypick-robot

@sahil143: once the present PR merges, I will cherry-pick it on top of release-4.4 in a new PR and assign it to you.

In response to this:

/cherrypick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-merge-robot openshift-merge-robot merged commit feb8b65 into openshift:master Mar 5, 2020
@openshift-ci-robot
Copy link
Contributor

@sahil143: All pull requests linked via external trackers have merged. Bugzilla bug 1806914 has been moved to the MODIFIED state.

In response to this:

Bug 1806914: redirect to topology when resource created from dev-catalog

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@openshift-cherrypick-robot

@sahil143: #4459 failed to apply on top of branch "release-4.4":

Using index info to reconstruct a base tree...
A	frontend/packages/operator-lifecycle-manager/src/components/create-operand-form.tsx
M	frontend/packages/operator-lifecycle-manager/src/components/create-operand.spec.tsx
M	frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx
Falling back to patching base and 3-way merge...
Auto-merging frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx
CONFLICT (content): Merge conflict in frontend/packages/operator-lifecycle-manager/src/components/create-operand.tsx
Auto-merging frontend/packages/operator-lifecycle-manager/src/components/create-operand.spec.tsx
CONFLICT (modify/delete): frontend/packages/operator-lifecycle-manager/src/components/create-operand-form.tsx deleted in HEAD and modified in redirect to topology when resource created from dev-catalog. Version redirect to topology when resource created from dev-catalog of frontend/packages/operator-lifecycle-manager/src/components/create-operand-form.tsx left in tree.
error: Failed to merge in the changes.
Patch failed at 0001 redirect to topology when resource created from dev-catalog

In response to this:

/cherrypick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@sahil143
Copy link
Contributor Author

sahil143 commented Mar 5, 2020

/cherrypick release-4.4

@openshift-cherrypick-robot

@sahil143: new pull request created: #4661

In response to this:

/cherrypick release-4.4

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository.

@spadgett spadgett added this to the v4.5 milestone Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. bugzilla/valid-bug Indicates that a referenced Bugzilla bug is valid for the branch this PR is targeting. component/core Related to console core functionality component/olm Related to OLM kind/bug Categorizes issue or PR as related to a bug. lgtm Indicates that a PR is ready to be merged. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants