Skip to content

Add kiali URL in traffic connector side panel #4026

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
Merged
openshift-merge-robot merged 2 commits into from
Jan 26, 2020
Merged

Add kiali URL in traffic connector side panel #4026

openshift-merge-robot merged 2 commits into from
Jan 26, 2020

Conversation

@divyanshiGupta
Copy link
Contributor

@divyanshiGupta divyanshiGupta commented Jan 21, 2020 •
edited
Loading

@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/XL Denotes a PR that changes 500-999 lines, ignoring generated files. component/dev-console Related to dev-console labels Jan 21, 2020
@jeff-phillips-18 jeff-phillips-18 mentioned this pull request Jan 21, 2020
Merged
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 22, 2020

/kind feature

@openshift-ci-robot openshift-ci-robot added the kind/feature Categorizes issue or PR as related to a new feature. label Jan 22, 2020
@christianvogt
Copy link
Contributor

christianvogt commented Jan 22, 2020

@divyanshiGupta have you seen the ConsoleLink updates coming in from kiali for creating a namespaced link from the dashboard?
https://github.com/kiali/kiali/pull/2078/files
Constructing the URL as you do isn't supported as API by kiali, while the ConsoleLink will be a valid URL. How different are the landing pages between the two urls? If it doesn't take us where we need to go, then constructing for now is ok as we work through the integration issues with using their internal apis.

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 22, 2020
@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Jan 22, 2020 •
edited
Loading

@divyanshiGupta have you seen the ConsoleLink updates coming in from kiali for creating a namespaced link from the dashboard?
https://github.com/kiali/kiali/pull/2078/files

I havent seen the updates since I wanted to get everything in place first and then according to ConsoleLink updates make changes to the PR.

Constructing the URL as you do isn't supported as API by kiali, while the ConsoleLink will be a valid URL. How different are the landing pages between the two urls? If it doesn't take us where we need to go, then constructing for now is ok as we work through the integration issues with using their internal apis.

I am using the ConsoleLink URL only, but I was under the impression there is going to be only one ConsoleLink with the root URL and we can append the path and query params we want along with the namespace. Now according to the PR they are creating ConsoleLink for each namespace that will have the graph view link filtered by namespace so if this URL without the params is same as what I created with params we can simply use the ConsoleLink URL directly without adding anything to it.

@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Jan 22, 2020 •
edited
Loading

@christianvogt both URLs will take us to the same page. So I will directly use only the consoleLink URL .

@divyanshiGupta divyanshiGupta changed the title [WIP]Add kiali URL in traffic connector side panel Add kiali URL in traffic connector side panel Jan 22, 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 22, 2020
@christianvogt
Copy link
Contributor

christianvogt commented Jan 22, 2020

Thanks @divyanshiGupta.
My concern is that their URLs aren't officially considered api for external manipulation. That being said it's a URL and bookmarks would break if they did change. So really all URLs should be considered API. But i can see their argument against.
So using the console link urls is an api they buy into.

Seems you're doing a lot of work to weave the console links data through topology. Why don't you simply connect to redux in the TopologyEdgePanel and create the url on the spot using the console links and namespace from redux?

@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Jan 22, 2020 •
edited
Loading

Seems you're doing a lot of work to weave the console links data through topology. Why don't you simply connect to redux in the TopologyEdgePanel and create the url on the spot using the console links and namespace from redux?

Because TopologyDataController is already a connected component and also I thought of passing the console-links from here since most of the data we need anywhere in topology is passed/controlled by this component. Just didn't wanted to break the flow. Also when we wanted to show sbr in sidepanel we added it as an edge data so similarly if we want to show kiali link in the sidepanel it can be considered as edge data and that way it will also be available across topology. Definitely making TopologyEdgePanel a connected component and getting the console-links would have been straightforward. What do you suggest? Also we don't need namespace now since looking at the PR you shared they are going to provide namespaced URL so we can directly use consoleLink URL.

@openshift-ci-robot openshift-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/kubevirt Related to kubevirt-plugin component/monitoring Related to monitoring size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jan 23, 2020
@divyanshiGupta
Copy link
Contributor Author

divyanshiGupta commented Jan 23, 2020

/assign @christianvogt

karthikjeeyar
karthikjeeyar reviewed Jan 23, 2020
View reviewed changes
@karthikjeeyar
Copy link
Contributor

karthikjeeyar commented Jan 24, 2020

verified the changes in e3b5c5d
/lgtm

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

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

Just reviewed the last commit - see comments below.

@openshift-ci-robot openshift-ci-robot removed the lgtm Indicates that a PR is ready to be merged. label Jan 24, 2020
andrewballantyne
andrewballantyne approved these changes Jan 24, 2020
View reviewed changes
Copy link
Contributor

@andrewballantyne andrewballantyne left a comment

Choose a reason for hiding this comment

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

/lgtm
/hold

Added my approval and re-added Karthik's lgtm. Put a hold on it until we can get the dependency PRs in.

@openshift-ci-robot openshift-ci-robot added do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. lgtm Indicates that a PR is ready to be merged. approved Indicates a PR has been approved by an approver from all required OWNERS files. labels Jan 24, 2020
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 24, 2020

/retest

1 similar comment
@spadgett
Copy link
Member

spadgett commented Jan 25, 2020

/retest

@openshift-ci-robot openshift-ci-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jan 25, 2020
sahil143 and others added 2 commits January 26, 2020 23:05
@sahil143 @divyanshiGupta
create a utility and component for traffic connector
e550a4e 
remove console logs

lint

add types for traffic data

add unit test for kiali utility

refactor

add test for truthy
@divyanshiGupta
Add kiali link to traffic connector side-panel
8e0720e
@openshift-ci-robot openshift-ci-robot removed lgtm Indicates that a PR is ready to be merged. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jan 26, 2020
@andrewballantyne
Copy link
Contributor

andrewballantyne commented Jan 26, 2020

/lgtm

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

openshift-ci-robot commented Jan 26, 2020

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: andrewballantyne, divyanshiGupta, karthikjeeyar

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

The pull request process is described here

Details 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

andrewballantyne commented Jan 26, 2020

/hold cancel

I'm going to throw all the chips on the table... If #4022 (the 1st commit of this PR & a dependency for this work) merges first, this PR will get conflicts and cannot merge... however if this merges before #4022 then 4022 won't be needed. We are having e2e failures that are blocking things and Sahil (author of 4022) will be on PTO during the next couple days preventing any response to failures / rebases / blockers that arise on #4022.

So, let the race begin 😄

@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 26, 2020
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

5 similar comments
@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

@openshift-bot
Copy link
Contributor

openshift-bot commented Jan 26, 2020

/retest

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

@openshift-merge-robot openshift-merge-robot merged commit 919f2db into openshift:master Jan 26, 2020
@sahil143 sahil143 mentioned this pull request Jan 27, 2020
Closed
@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

@nemesis09 nemesis09 Awaiting requested review from nemesis09

2 more reviewers

@karthikjeeyar karthikjeeyar karthikjeeyar left review comments

@andrewballantyne andrewballantyne andrewballantyne approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@andrewballantyne andrewballantyne

@karthikjeeyar karthikjeeyar

@christianvogt christianvogt

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. component/ceph Related to ceph-storage-plugin component/core Related to console core functionality component/dev-console Related to dev-console component/kubevirt Related to kubevirt-plugin component/monitoring Related to monitoring kind/feature Categorizes issue or PR as related to a new feature. lgtm Indicates that a PR is ready to be merged. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files.

Projects

None yet

Milestone

v4.4

Development

Successfully merging this pull request may close these issues.

9 participants

@divyanshiGupta @andrewballantyne @christianvogt @karthikjeeyar @spadgett @openshift-ci-robot @openshift-bot @openshift-merge-robot @sahil143