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

area: ui #1443

Conversation

sergeyglazyrindev
Copy link

@sergeyglazyrindev sergeyglazyrindev commented Nov 5, 2018

Hello guys
Here is an updated gui for skydive
Few things, mostly minor need to be fixed and I hope to finalize these things on this week.
you may start review this changeset and in the end of week I'll ask you to start using this new gui
There's a safe way to try out this ui. Just add to the query string: use_hardcoded_data=1&newui_approach=1

An example: http://192.168.56.101:8082/topology?use_hardcoded_data=1&newui_approach=1
If you want to try it out on fresh topology, remove use_hardcoded_data=1 from query string

skip skydive-go-fmt
skip skydive-functional-tests-backend-elasticsearch
skip skydive-functional-tests-backend-orientdb
skip skydive-scale-tests
skip skydive-unit-tests
skip skydive-compile-tests
skip skydive-k8s-tests
skip skydive-ppc64le-tests

@safchain
Copy link
Collaborator

safchain commented Nov 6, 2018

@sergeyglazyrindev Thanks for submitting this, Indeed that is a huge PR :) Few first remarks about the approach and not the implementation.

We should remove the generated files from the commit (JS) using the makefile to generate them. We don't need to have them in the repo and will ease the review process.

I worked on a patch to generate the REST API using Webpack/TS. I'll submit a draft by tomorrow so that we could use the same build process.

The REST API generated and published here should be used https://www.npmjs.com/search?q=skydive I also converted the Websocket API using "real" class for nodes and edges. Everything compiles using strict option.

We need also to have the functional test passing "cdd_overview"

@sergeyglazyrindev
Copy link
Author

@safchain , I'll update make file when fix all issues and remove generated file

@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch from 07dd2c6 to 2135a98 Compare November 7, 2018 09:38
@sergeyglazyrindev
Copy link
Author

The remaining things are:

  1. introduce a new layout which will be used for filtering the infrastructure topology using gremlin queries, etc
  2. introduce one websocket per layout, get rid of global websocket handler
  3. fix realtime updates in data structures
  4. test link labels bandwidth for links, latencies, etc
  5. test onNodeAdded and onEdgeAdded events and make sure emphasize handler works properly
  6. test highlight and unhighlight events
  7. fix all remaining @todo in the code (not so much)
  8. do final tests

@sergeyglazyrindev
Copy link
Author

Btw, do we have a typescript linter in our build process ? @lebauce , I think that makes sense to introduce such thing into our build process.

@sergeyglazyrindev
Copy link
Author

Btw, do we have a typescript linter in our build process ? @lebauce , I think that makes sense to introduce such thing into our build process.

I can do that but later.....

@safchain
Copy link
Collaborator

safchain commented Nov 7, 2018

@sergeyglazyrindev I working on adding typescript/webpack build in the build process so I can add the linter thing

@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch 3 times, most recently from 04a77e9 to 8b3160a Compare November 8, 2018 13:52
@sergeyglazyrindev
Copy link
Author

hello guys.
so the remaining things are:

  1. fix typescript linter warnings
  2. do final tests
  3. integrate process of building topology package into makefile.
  4. make sure all checks passed.

I think the changeset is ready for testing.
Unfortunately, I had to remove old way of building graph because usage of this global object: topologyComponent and graph made almost impossible to do a step by step integration of new approach.

How do we test it ? I made enough tests to make sure it works but I've not tested workflows, rule-detail component, there are some communication with graph/topologyComponent.

Tomorrow I'll fix typescript linter warnings.

@sergeyglazyrindev
Copy link
Author

for sure there things which could be improved but right now I want to make a smooth transition from old approach to a new one. and then we can improve, write tests, actually, right now there's no tests for new approach because this stuff I developed from scratch during last two weeks. but we have one another stuff which has tests, etc: hierarchied topology
we may contribute it but only after this new approach would be merged because support of different layouts in skydive now very problematic.

@sergeyglazyrindev
Copy link
Author

Later on I'll create a npm package to get rid of this topology stuff in skydive repo.

@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch 17 times, most recently from e82e027 to 9019a54 Compare November 12, 2018 02:26
+ fixed websocket connection issues
endless websocket connections if connection dropped,
reinitialize layout properly
introduced a parameter of datasource: reconnectmode - automatic or not
fixed capture-form in original js implementation
fixed inject form in original js implementation
fixed node selector in original js implementation
Change-Id: I287adb2702acfbe401d833a6ff3522ac4b0ffb3b
Partial-Bug: #SOFTID-201
Signed-off-by: Sergey Glazyrin <s.glazyrin@partner.samsung.com>
@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch 16 times, most recently from f6f4b2a to 6f330fd Compare November 20, 2018 21:24
@sergeyglazyrindev
Copy link
Author

run skydive-cdd-overview-tests

@sergeyglazyrindev
Copy link
Author

hello @safchain .
I returned old implementation of skydive UI and made some bridge between new UI and old UI so now it's easier to test at the same time both approaches.
You may test it using following link: http://SKYDIVE_ANALYZER_IP:8082/topology?test_newui=1
if you want to use old UI, remove test_newui=1 from query string.

So the remaining things are:

  • fix typescript linter warnings
  • refactor typescript code, its not ideal now
  • integrate process of building topology package into makefile.

I'll solve these things when we merge this changeset, right now it's very huge changeset
Later on this file statics/js/components/old_topology_approach.js will be removed. There is everything which related to old approach

@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch from 22a5a7d to f97854c Compare November 20, 2018 22:31
now its possible to test a new layout but by
default enabled old layout
Partial-Bug: #SOFTID-201

Change-Id: I12ace0fe4464d658d22874b222e85233e80922d9
Signed-off-by: Sergey Glazyrin <s.glazyrin@partner.samsung.com>
@sergeyglazyrindev sergeyglazyrindev force-pushed the samsung-contrib-ui-package-package-v2 branch from f97854c to 5839c0c Compare November 20, 2018 22:51
@sergeyglazyrindev
Copy link
Author

hello @safchain
You can review, but it provides a way to use old ui and new at the same time...
Is there any chance to reduce the risks of merging of this changeset into the master ?
I fixed nothing in the cdd_tests, so I think default ui should work properly.
And then we can slowly test new approach, improve, etc
What do you think ?

@safchain
Copy link
Collaborator

@sergeyglazyrindev I'll have another look. As it is a huge patch it will take me a bit of time to be comfortable with it :). So first I'll have an another look at the global approach.

@sergeyglazyrindev
Copy link
Author

right now I added a solution to support both versions of UI. I think old one works ok, I changed nothing. Tests are working. The only one thing, I moved all files which are supposed to be deleted to file: old_topology_approach.js
Later on when we migrate to new approach fully, we can remove just this file....

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.

None yet

3 participants