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

feat(matchExpr): match expression visualizer #914

Merged
merged 11 commits into from
Mar 22, 2023

Conversation

tthvo
Copy link
Member

@tthvo tthvo commented Mar 20, 2023

Welcome to Cryostat! 👋

Before contributing, make sure you have:

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

Fixes: #911
Fixes: #912
Fixes: #900

Description of the change:

  • Match Expression Visualizer with mini topology
    • Graph View
    • List view (if # of targets are too large).
    • Fix height constraints -> need to fill viewport instead of overflowing.
    • Update target detail panel to reflect fieldName.

Note: No tests yet tho. Focusing on flushing out features for now.

Motivation for the change:

See #376 (comment)

How to manually test:

  1. Run CRYOSTAT_IMAGE=quay.io... sh smoketest.sh...
  2. Go to Rule and Credential Forms.

Screenshots

Credential Modal

Screenshot from 2023-03-20 23-30-29

Create Rule Form

Screenshot from 2023-03-21 01-25-39

Match Expression Hint

Screenshot from 2023-03-21 01-27-44

@tthvo tthvo added the feat New feature or request label Mar 20, 2023
@mergify mergify bot added the safe-to-test label Mar 20, 2023
@tthvo tthvo changed the title feat(matchExpr): topology-based match expression evaluator feat(matchExpr): match expression visualizer Mar 20, 2023
@tthvo tthvo force-pushed the match-expr-vis branch 3 times, most recently from 18bfa62 to e4e0b0c Compare March 21, 2023 04:10
@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Notice how the field path is represented as breadcrumbs. I took this design from OpenShift console. Let me know what you think :D

Screenshot from 2023-03-20 23-36-40

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-79dd59579b23171d9228e0e64df803b3862e42e2 sh smoketest.sh

@cryostatio cryostatio deleted a comment from github-actions bot Mar 21, 2023
@cryostatio cryostatio deleted a comment from github-actions bot Mar 21, 2023
@cryostatio cryostatio deleted a comment from github-actions bot Mar 21, 2023
@cryostatio cryostatio deleted a comment from github-actions bot Mar 21, 2023
@tthvo tthvo force-pushed the match-expr-vis branch 2 times, most recently from 66f60d7 to 12e0ad3 Compare March 21, 2023 18:12
@tthvo tthvo marked this pull request as ready for review March 21, 2023 18:13
@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Read for review now :D

Just a small note for testing with observables that includes debounceTime: just need to use waitFor to check if some function (e.g. api requests, etc) is called. Otherwise, the test will move on before the operation completes.

Also, you might notice that the graph view sometimes flicks on first renders. This is because I have an effect that tries to fit all nodes into views. So, there is some animation there but too fast to see.

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-12e0ad30549eefc5b6e28c17f9abf079da9e829b sh smoketest.sh

@andrewazores
Copy link
Member

  1. Go to Topology
  2. Select a target with the warning exclamation
  3. Click "Add Credentials"
  4. Select a target with the warning exclamation
  5. Click "Add Credentials" ...

Can that action be removed from the sidebar panel component when it's in this visualization context?

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Yeh sure, of course!

I left it in case the user want to add credentials in Create Rule Form. For example. if the target is matched but could not fetch templates, adding credentials there can help refreshing on the go. What do you think?

But yeh it really still needs removing when for example, on a credential modal.

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

I added options to allow hide action buttons. This is applied for credential modal but still kept for Create Rule Form.

Screenshot from 2023-03-21 15-55-15

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Views appear not responding to target discovery events when adding credentials (i.e. create form rule). Investigating...

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-5973510780907c18111a2fb0daeebdb0013fdd07 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-14687f6b76d677d6678c1b95c08616578880a56f sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-3ae8348722b08acb9f2b095db42b062277330e5f sh smoketest.sh

@andrewazores
Copy link
Member

Yea, the Actions menu within the sidecar panel within the modal. Nothing happens when I click it. The "outer" one, in the Topology view, has actions when I click it.

@andrewazores
Copy link
Member

Also the text at the top of that modal needs updating:

Creates stored credentials for target JVMs according to various properties. If a Target JVM requires JMX authentication, Cryostat will use stored credentials when attempting to open JMX connections to the target.

Should be more generic and just talk about opening connections, not specifically JMX ones.

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Ohhh interesting...there is a bug somewhere I think. It should show up. Let me check!

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-57a204e4de6f9b72aac06faf58cec53e742f20a9 sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

Never mind, the menu was actually showing but it was mounted in the back of the modal haha. Fixed now :D

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-56c320e0a136f1ad8a412520cb168be0d3d7a9dc sh smoketest.sh

@tthvo
Copy link
Member Author

tthvo commented Mar 21, 2023

  • Updated texts.
  • Removed actions in empty state: asynchronously update input field will cause the cursor jump to the end -> cant use hook -> actions to reset match expression can't be used. However, the the match expression input field is already visible to edit.

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-17e0bb547ab6c26ae0a4ad3e4a2573c459e75a41 sh smoketest.sh

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-3ed4d7caa13a7525b7af394f1000b8540575dd4b sh smoketest.sh

@andrewazores
Copy link
Member

Looks good testing it out. I'll review the implementation later this afternoon.

@maxcao13
Copy link
Member

image
Should the message be different when the expression is valid?

@maxcao13
Copy link
Member

Screencast.from.03-22-2023.03.15.13.PM.webm

If it's not hard to do, keep the positions of the nodes when switching between graph view and list view?

@tthvo
Copy link
Member Author

tthvo commented Mar 22, 2023

If it's not hard to do, keep the positions of the nodes when switching between graph view and list view?

Should be okay to do I think. Just that this might cause another huge chunk of data to be stored if # of targets is large. Especially, topology view also saves positions too.

Should the message be different when the expression is valid?

https://www.patternfly.org/v4/components/form/design-guidelines

No guideline for valid message... I think the green color + check icon should be enough to indicate that.

@tthvo
Copy link
Member Author

tthvo commented Mar 22, 2023

Updated to to save graph positions and clean up css, console.logs :))

@github-actions
Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-914-ff12a36baf68e67a26c2abbd7f472aa8837efe07 sh smoketest.sh

Copy link
Member

@maxcao13 maxcao13 left a comment

Choose a reason for hiding this comment

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

Looks great!

@andrewazores andrewazores merged commit 547a37d into cryostatio:main Mar 22, 2023
@tthvo tthvo deleted the match-expr-vis branch March 22, 2023 22:44
@tthvo tthvo mentioned this pull request Mar 23, 2023
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request safe-to-test
Projects
No open projects
Status: Done
3 participants