cryostatio / cryostat-web

Web front-end for Cryostat: Secure JDK Flight Recorder management for containerized JVMs

https://cryostat.io/

Other

10 stars 20 forks source link

feat(topology): add target.Agent topology filter #1339

Closed mwangggg closed 1 week ago

mwangggg commented 2 weeks ago

Welcome to Cryostat! 👋

Before contributing, make sure you have:

[x] Read the contributing guidelines
[x] Linked a relevant issue which this PR resolves
[x] Linked any other relevant issues, PR's, or documentation, if any
[x] Resolved all conflicts, if any
[x] Rebased your branch PR on top of the latest upstream main branch
[x] Attached at least one of the following labels to the PR: [chore, ci, docs, feat, fix, test]
[x] Signed all commits using a GPG signature

To recreate commits with GPG signature git fetch upstream && git rebase --force --gpg-sign upstream/main

Fixes: #1325

andrewazores commented 2 weeks ago

The target.agent property is basically just shorthand for target.connectUrl.startsWith('http'), so I'm a little conflicted on whether it deserves its own separate list item. But, if we don't list it there, then users may not realize it's available to reference in their expressions.

tthvo commented 2 weeks ago

The target.agent property is basically just shorthand for target.connectUrl.startsWith('http'), so I'm a little conflicted on whether it deserves its own separate list item. But, if we don't list it there, then users may not realize it's available to reference in their expressions.

Ah right, how about just an Info icon that shows next to the connectUrl of an Agent-based taget? This is compact and also gives an example expression. A small diff here (using Patternfly 5, PF4 might look a bit different): https://gist.github.com/tthvo/2c05f789cccbb759745bd3ec4ddee806

For the connectUrl filters, we can categorize them based on whether they are Agent-based:

andrewazores commented 2 weeks ago

Sure, I think that looks like a nice middle ground design approach.

tthvo commented 1 week ago

Quick question tho @andrewazores @mwangggg . Should this target pf5 branch? I am hoping the upgrade will finish in the next 1-2 weeks or so. Just that there might be a huge conflicts to resolve after merge :D

andrewazores commented 1 week ago

I'm fine with that.

I'll go ahead and merge this into main for now so that it doesn't just sit idle doing nothing, then either rebase pf5 onto main or merge main into pf5, and feel free to update/alter the new desig in another PR targeting the pf5 branch.