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

add --watch support for resource watching #113

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

jasonmadigan
Copy link
Member

@jasonmadigan jasonmadigan commented Jan 28, 2025

Started working on #111

But then ended up straying into: #110 as well

Summary:

  • Added a --watch flag that setups a watcher for a topology ConfigMap
  • Renamed --output / -o to --svg / -s (made more sense in my head, output seemed vague?)
  • Can use combos of --ws / --svg / --dot. If these are combined with --watch, updates are watched for and the SVG / DOT files are updated until sigint

Verifying:

Checkout locally, and go build.

Example commands:

./kuadrantctl topology --svg topology.svg --watch # SVG that updates on topology changes
./kuadrantctl topology --svg topology.svg # Just give me a one and done SVG
./kuadrantctl topology --watch --svg topology.svg --dot topology.dot # Give me everything, SVG, DOTfile + WS (implicit watch)
./kuadrantctl topology --watch  --dot topology.dot # WebSocket only (implicit --watch)

@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch from 8287bf9 to a3aeacc Compare January 28, 2025 16:09
@jasonmadigan
Copy link
Member Author

@eguzki spent a little time on this today, and learned some stuff about goroutines. Would appreciate your thoughts (not done yet).

@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch 6 times, most recently from 69daba0 to 671c604 Compare January 28, 2025 17:15
@jasonmadigan
Copy link
Member Author

jasonmadigan commented Jan 29, 2025

test

early draft of this working with the web ui: Kuadrant/react-policy-topology#7

next: add the --web flag.

At some point, we'll update react-policy-topology to use PF and work more like the console plugin's topology view

@jasonmadigan
Copy link
Member Author

Wondering about options to run a web-app container:

  • use github.com/docker/docker/client etc to interact with docker. One issue: want this to work with podman too. I know podman has a docker-compatible restful API, but I'm not 100% sure if this works OOTB.
  • shell out to whatever runtime is available. Easy, but lots of messiness to deal with that I don't think really belongs in kuadrantctl
  • find a nice golang abstraction regardless container runtimes - this would be ideal

Copy link
Collaborator

@eguzki eguzki left a comment

Choose a reason for hiding this comment

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

First of all, loving this idea.

Let me discuss the implementation at high level with few questions.

Have you considered running the web server from within another goroutine that is listening to some channel for updates? The service would serve a react application that would open websockets locally waiting for topology updates. Hopefully this makes some sense.

The term websocket is an implementation detail IMO. The user only needs to know that the tool can display the topology in a SVG viewer (whatever the OS has a default viewer for .svg files) or run a local webserver and use your browser to display the topology.

We can always go with docker approach, but that looks like heavier approach, starting with the user is required to have docker installed.

Last, the code looks good and by no means I am asking for a big refactor. At the same time, I would like to say that there is room for improvement and have more encapsulated code. I see like two main functionalities, mutually exclusive, that can be encapsulated separately. One is when the user wants to display the topology using SVG viewer and the other one is running some local server (again websockets is the implementation detail for me) and display using the web browser.

cmd/topology.go Outdated Show resolved Hide resolved
cmd/topology.go Show resolved Hide resolved
cmd/topology.go Outdated Show resolved Hide resolved
cmd/topology.go Outdated Show resolved Hide resolved
@jasonmadigan
Copy link
Member Author

jasonmadigan commented Feb 6, 2025

Couple of changes TODO (will find time this week):

  • remove the --ws + websocket serving stuff
  • keep the watch stuff, still useful for dynamic svg/dotfile updates.
  • Local users: open the DOTfile in something like vscode or whatever, and with --watch, it'll dynamically update

we'll separately look at a deployable service for kube to do similar to the WS stuff (Basically: have an installable service for a topology service on kube/podman/docker)

@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch from 671c604 to 03cbec6 Compare February 7, 2025 09:28
@jasonmadigan jasonmadigan changed the title draft: add support for resource watching, and websocket output draft: add support for resource watching Feb 7, 2025
@jasonmadigan jasonmadigan changed the title draft: add support for resource watching add --watch support for resource watching Feb 7, 2025
@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch 2 times, most recently from d7f4a1d to 42a0f70 Compare February 7, 2025 11:22
@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch from 42a0f70 to 59d84ea Compare February 7, 2025 11:42
@jasonmadigan jasonmadigan force-pushed the 111-websocket-web-support branch from 59d84ea to 11e7ee0 Compare February 7, 2025 11:46
@jasonmadigan jasonmadigan marked this pull request as ready for review February 7, 2025 11:46
@jasonmadigan
Copy link
Member Author

@eguzki think this is ready for another pass, have paired this back to just adding --watch for the time being

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.

3 participants