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

fix(matchexpr): tooltip hint checks for annotation existence before asserting value #1275

Merged
merged 2 commits into from
Jun 11, 2024

Conversation

andrewazores
Copy link
Member

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 all commits using a GPG signature

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


Related to cryostatio/cryostat#511

Description of the change:

Adds a check that the annotation exists before reading it to compare its value.

Motivation for the change:

In the old JavaScript-based expression evaluation, indexing the map with a nonexistent key would simply result in an undefined value which would compare as false against the test value. In CEL it is a runtime error to index with a nonexistent key, so there should be an existence check before the access and comparison to ensure that the result comes back as a successful evaluation with a false result, rather than as an error.

How to manually test:

  1. Check out cryostat3 with this PR and build
  2. ./smoketest.bash -Ot
  3. Go to Automated Rules > Create, click a target in the miniature topology view
  4. Click the match expression tooltip ? icon. Copy the suggested expression and paste it into the match expression box.
  5. If the suggested annotation exists only on some targets and not others, then the evaluation would fail prior to this PR.

@andrewazores
Copy link
Member Author

/build_test

Copy link

Test image available:

CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat-web:pr-1275-fd4ab0fa244c7df32dc6a9acc9c721512fd3cb1a bash smoketest.bash # then open http://localhost:8080

@andrewazores andrewazores requested a review from ebaron June 11, 2024 14:02
Copy link
Member

@ebaron ebaron left a comment

Choose a reason for hiding this comment

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

Works for me

@ebaron ebaron merged commit 7ce8ee4 into cryostatio:main Jun 11, 2024
18 checks passed
mergify bot pushed a commit that referenced this pull request Jun 11, 2024
…sserting value (#1275)

* fix(matchexpr): tooltip hint checks for annotation existence before asserting value

* formatting

(cherry picked from commit 7ce8ee4)
andrewazores added a commit that referenced this pull request Jun 11, 2024
…sserting value (#1275) (#1280)

(cherry picked from commit 7ce8ee4)

Co-authored-by: Andrew Azores <aazores@redhat.com>
@andrewazores andrewazores deleted the matchexpr-tooltip branch June 11, 2024 18:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants