-
Notifications
You must be signed in to change notification settings - Fork 10
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(recordingOptions): scope customizers to each Target #341
fix(recordingOptions): scope customizers to each Target #341
Conversation
edf1278
to
7b1584f
Compare
/build_test |
Workflow started at 3/22/2024, 2:30:45 PM. View Actions Run. |
CI build and push: All tests pass ✅ (JDK21) |
No OpenAPI schema changes detected. |
7b1584f
to
5ca347e
Compare
CI build and push: All tests pass ✅ (JDK17) |
5ca347e
to
7ff32db
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rebase needed- looks good otherwise
7ff32db
to
31ed317
Compare
/build_test |
Workflow started at 4/15/2024, 9:20:25 AM. View Actions Run. |
No OpenAPI schema changes detected. |
CI build and push: All tests pass ✅ (JDK17) |
Welcome to Cryostat3! 👋
Before contributing, make sure you have:
main
branch[chore, ci, docs, feat, fix, test]
To recreate commits with GPG signature
git fetch upstream && git rebase --force --gpg-sign upstream/main
Fixes: #184
Description of the change:
Refactors the
RecordingOptionsCustomizer
implementation so that there is an internal in-memory cacheMap<Target, RecordingOptionsCustomizer>
. This way, thePATCH /api/v1/:connectUrl/recordingOptions
request can set recording options defaults (for the "advanced options" ofmaxAge
/maxSize
/toDisk
) for each target. Cache entries are evicted if the target is lost. These settings are only held in-memory and not persisted.Motivation for the change:
Usually, clients do not actually set defaults on targets like this, instead simply setting these options on each recording in the same request as creating the recording. However, the API does expose this endpoint for setting per-target defaults, and so the implementation should actually respect this and ensure that the defaults are per-target rather than global to the server.
How to manually test:
PATCH
endpoint.However, using
curl
orhttp
you can make a manual request to thisPATCH
and the correspondingGET
to see that the changes are reflected. If you do make such aPATCH
then the default settings displayed in the UI under Recordings > Create should match the updated defaults you set.This would previously already work, however the difference is that the updated
PATCH
settings would previously end up applying to all targets (despite the connectUrl or targetId in the request path), whereas now they apply to individual targets.