-
Notifications
You must be signed in to change notification settings - Fork 14
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
draft: add support for resource watching, and websocket output #113
base: main
Are you sure you want to change the base?
Conversation
8287bf9
to
a3aeacc
Compare
@eguzki spent a little time on this today, and learned some stuff about goroutines. Would appreciate your thoughts (not done yet). |
3dabad4
to
69daba0
Compare
…ebSocket support Signed-off-by: Jason Madigan <jason@jasonmadigan.com>
69daba0
to
671c604
Compare
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 |
Wondering about options to run a web-app container:
|
There was a problem hiding this 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.Flags().BoolVar(&watchFlag, "watch", false, "Enable resource watching for continuous updates") | ||
cmd.Flags().BoolVarP(&websocketFlag, "ws", "w", false, "Start a WebSocket server to stream topology updates") | ||
cmd.Flags().StringVar(&wsHost, "ws-host", "localhost", "WebSocket server hostname") | ||
cmd.Flags().IntVar(&wsPort, "ws-port", 4000, "WebSocket server port") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice! allows to override the port in case the user is running something else locally in the same port.
|
||
var mgr manager.Manager | ||
if watchFlag { | ||
mgr, err = manager.New(configuration, manager.Options{ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Turning kuadrantctl topology
into a k8s controller :) Nice!
|
||
func (w *configMapWatcher) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) { | ||
configMap := &corev1.ConfigMap{} | ||
if err := w.Get(ctx, req.NamespacedName, configMap); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if the topology is not found, that is a known error you can check and exit without any error. Returning an error would make the controller to retry
} | ||
|
||
if configMap.Name != "topology" || configMap.Namespace != topologyNS { | ||
return ctrl.Result{}, nil |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: this is being defined by the predicate. I would just panic so, if for some reason, the runtime goes through that path, the tool is very clear that this is unexpected.
Started working on #111
But then ended up straying into: #110 as well
Summary:
--watch
flag that setups a watcher for a topologyConfigMap
--ws
flag that sets up a local WebSocket server to publish initial topology (and subsequent updates)--output / -o
to--svg / -s
(made more sense in my head,output
seemed vague?)--ws
/--svg
/--dot
. If these are combined with--watch
, updates are watched for and the SVG / DOT files are updated until sigint. Note:--watch
becomes implicit if using--ws
, as starting a WS to flush just once response seemed a bit silly.Verifying:
Checkout locally, and
go build
.Example commands:
TODO:
--web
to start a web-server to serve up a simple web page to be fed by the WebSocket, based on https://github.com/Kuadrant/react-policy-topology