Skip to content
This repository was archived by the owner on Apr 11, 2023. It is now read-only.

KIALI-1906 Support for security and non security environments#1008

Merged
aljesusg merged 9 commits intokiali:masterfrom
aljesusg:support_cases_jaeger
Feb 26, 2019
Merged

KIALI-1906 Support for security and non security environments#1008
aljesusg merged 9 commits intokiali:masterfrom
aljesusg:support_cases_jaeger

Conversation

@aljesusg
Copy link
Copy Markdown
Contributor

With this we support the integration of Jaeger with Iframe when:

  • kiali http and Jaeger http
  • kiali https and Jaeger https

When one of them is with security and the other without we'll have the link to the jaeger website.

What do you think @lucasponce, @jotak ?

With this we only need to work in the JAEGER_URL provided by the backend or Makefile

@pilhuhn
Copy link
Copy Markdown
Contributor

pilhuhn commented Feb 20, 2019

Sounds fair.

@aljesusg aljesusg mentioned this pull request Feb 20, 2019
25 tasks
@aljesusg aljesusg added the do not merge A PR is not ready to merge label Feb 20, 2019
@aljesusg
Copy link
Copy Markdown
Contributor Author

Hey @jotak I did the changes requested in the jaeger integration PR, can you review it?

@aljesusg aljesusg requested a review from jotak February 21, 2019 12:37
* Paths
*/

export enum paths {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Oops, wrong one, enums are PascalCase ;-)
https://github.com/basarat/typescript-book/blob/master/docs/styleguide/styleguide.md#enum

I think this file was good as it was before, also regarding its name (Path.ts) and the enum values (for which we decided to keep upper case)

status: Status;
items: string[];
targetPage: Paths;
targetPage: string;
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why remove Paths? It's more specific typing, so better.

@aljesusg aljesusg removed the do not merge A PR is not ready to merge label Feb 22, 2019
@jotak
Copy link
Copy Markdown
Contributor

jotak commented Feb 25, 2019

@aljesusg there's just a missing comment from the previous PR, not addressed yet:

https://github.com/kiali/kiali-ui/pull/965/files#r258911600 : we don't use ^ for semver in all other modules

@aljesusg
Copy link
Copy Markdown
Contributor Author

@aljesusg there's just a missing comment from the previous PR, not addressed yet:

https://github.com/kiali/kiali-ui/pull/965/files#r258911600 : we don't use ^ for semver in all other modules

Changed Thanks

@aljesusg aljesusg force-pushed the support_cases_jaeger branch from f98acab to 708629e Compare February 26, 2019 09:19
@jotak
Copy link
Copy Markdown
Contributor

jotak commented Feb 26, 2019

So I tested these scenarios:

  • Kiali on https and Jaeger on http: the tab opens Jaeger UI in a new window
  • Kiali and Jaeger both on https: the tab shows Jaeger embedded

So I think this is what's expected.
The only suggestion I have would be to add an "external link" icon in the case it's opened in a new window (fa-external-link), so that there's no surprise for the user.

@aljesusg
Copy link
Copy Markdown
Contributor Author

aljesusg commented Feb 26, 2019

Done @jotak
external

My concern is about the link of Distributed Tracing Link that opens jaeger in a new tab too

@jotak
Copy link
Copy Markdown
Contributor

jotak commented Feb 26, 2019

@aljesusg yes there's the same problem with the distributed tracing link. But at least, it doesn't affect the nominal case where both jaeger and kiali are under https. I would say it's good enough.

Copy link
Copy Markdown
Contributor

@jotak jotak left a comment

Choose a reason for hiding this comment

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

LGTM

@aljesusg aljesusg merged commit a697303 into kiali:master Feb 26, 2019
@aljesusg aljesusg deleted the support_cases_jaeger branch February 26, 2019 12:17
@hhovsepy
Copy link
Copy Markdown
Contributor

When refreshing the Service Details page, it first appears "Traces" tab with external link, then within an 1/2 sec, switches to "Error Traces" tab.

@aljesusg
Copy link
Copy Markdown
Contributor Author

This is by the redux state I think let me check it @hhovsepy

@aljesusg
Copy link
Copy Markdown
Contributor Author

@hhovsepy I'll fix this problems in #1025 see comment #1025 (comment) Thanks

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants