cryostatio / cryostat

Secure JDK Flight Recorder management for containerized JVMs
https://cryostat.io
Other
11 stars 9 forks source link

fix(template): correct return of String-encoded Optional #482

Closed andrewazores closed 4 months ago

andrewazores commented 4 months ago

Welcome to Cryostat3! 👋

Before contributing, make sure you have:

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


Fixes: #481

Description of the change:

There was a bit of jumbled handling of an Optional in the implementation from #449, which resulted in an Optional that wrapped around another Optional#toString() call erroneously. The original Optional itself is the expected return, not its String form.

This got missed because the test doesn't run in CI. I think this is because of the Maven property to not build container images during CI. This probably made sense previously when there were no real tests to run and so building the container image as a throwaway was just a waste of CI time, but now there are integration tests to be run which do need the container image to have been built first.

How to manually test:

  1. Check out PR
  2. ./mvnw clean verify ; podman image prune -f. All tests should pass
  3. ./smoketest.bash -O, once the test comes up open the UI and start a recording on Cryostat itself using localhost:0 using the Cryostat template. Go to Security and define a stored credential that matches cryostat3:9091 with cryostat:password, create another stored credential with false and test:test, go back to Recordings and select cryostat3:9091. Then download the recording you started and open it with JMC. Go to the Event Browser and verify that the events were captured.

Screenshot_2024-05-29_16-08-06

andrewazores commented 4 months ago

/build_test

github-actions[bot] commented 4 months ago

Workflow started at 5/29/2024, 3:20:05 PM. View Actions Run.

github-actions[bot] commented 4 months ago

No OpenAPI schema changes detected.

github-actions[bot] commented 4 months ago

No GraphQL schema changes detected.

github-actions[bot] commented 4 months ago

CI build and push: At least one test failed ❌ (JDK17) https://github.com/cryostatio/cryostat3/actions/runs/9291897968

andrewazores commented 4 months ago

I'm not sure what's up with the CI failure. It got to the point of running the test, but the test failed because the expected event template wasn't found. I'm not sure how that can be the case. This test passes locally, and I can't remember the exact reason I initially had this test disabled in CI only - I suppose it was also failing in CI, but I don't quite understand what that is the case. Maybe it has to do with the fact that the CI action passes the Maven property to not build a container image, but I would expect that to either cause the tests not to run or to run with an outdated image, and even a very outdated image should still contain that template file.

https://github.com/cryostatio/cryostat3/blame/main/src/test/java/itest/CryostatTemplateIT.java#L35

https://github.com/cryostatio/cryostat3/pull/151

andrewazores commented 4 months ago

/build_test

github-actions[bot] commented 4 months ago

Workflow started at 5/29/2024, 4:09:17 PM. View Actions Run.

github-actions[bot] commented 4 months ago

No GraphQL schema changes detected.

github-actions[bot] commented 4 months ago

No OpenAPI schema changes detected.

github-actions[bot] commented 4 months ago

CI build and push: At least one test failed ❌ (JDK17) https://github.com/cryostatio/cryostat3/actions/runs/9292460096

andrewazores commented 4 months ago

./mvnw -Dquarkus.container-image.build=true clean verify ; podman image prune -f fails locally.

./mvnw -Dquarkus.container-image.build=false clean verify ; podman image prune -f or ./mvnw clean verify ; podman image prune -f passes.

It seems like if the image doesn't get built then the tests run with a local process version of Cryostat. The cryostat.jfc file that the test is looking for gets inserted as an additional asset at container build time, so in this scenario it's expected that the template is not present. If the container does get built then the test runner runs using that, which does contain the template.

So once this is merged and the CI action change takes effect this should pass in CI as well.