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(api): fix GET /api serialization and webserver JFR event #1193

Merged
merged 9 commits into from
Nov 7, 2022

Conversation

andrewazores
Copy link
Member

Fixes #1191
Fixes #1192

  • fix(api): GET /api returns properly serialized endpoint verb
  • chore(log): replace request logging with Vertx impl
  • correct event name and move into logging handler

@andrewazores
Copy link
Member Author

Marked with backport label though I think it's too late to get this in to 2.2.0. These are lower-priority fixes solving problems that most users won't run into so if a 2.2.1 happens then this can get pulled in.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1193-57b0802cf2c57afec86de5f5ee11c168381d654f 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.

Everything looks good to me.

Should there be a test suite for RequestLoggingHandler?
For example testing the basic API specs, and that a WebServerRequest event is really emitted during a handling of some fake RequestHandler perhaps.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1193-b396e9e991560a50023cc33d4c5e1288142a13fd sh smoketest.sh

maxcao13
maxcao13 previously approved these changes Nov 4, 2022
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.

LGTM

tthvo
tthvo previously approved these changes Nov 4, 2022
Copy link
Member

@tthvo tthvo 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 to me :D

@andrewazores
Copy link
Member Author

Marked with backport label though I think it's too late to get this in to 2.2.0. These are lower-priority fixes solving problems that most users won't run into so if a 2.2.1 happens then this can get pulled in.

Actually... this would end up in the upstream 2.2.0 but it's too late for the downstream 2.2.0. Doesn't seem to make sense. I'll remove the label.

@github-actions
Copy link
Contributor

github-actions bot commented Nov 4, 2022

Test image available:

$ CRYOSTAT_IMAGE=ghcr.io/cryostatio/cryostat:pr-1193-db3cc5bd9477a8abfa9aa87648f75eddad17d96a sh smoketest.sh

@andrewazores andrewazores merged commit d1d45f6 into cryostatio:main Nov 7, 2022
@andrewazores andrewazores deleted the api-cleanup branch November 7, 2022 14:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
3 participants