-
Notifications
You must be signed in to change notification settings - Fork 31
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
New rule definitions trigger on existing targets #538
New rule definitions trigger on existing targets #538
Conversation
70a3753
to
40fc7c4
Compare
Checking my understanding: Is the purpose of the |
Pretty much, yes. The API handler for creating recordings used to process the POST request and form and then also handle all of the actual logic of creating the recording. It made sense to extract that out to somewhere reusable so that automatic rules can perform the same logic, but without having to do an HTTP request to ourselves and deal with a nested request/response (client -> cryostat -> cryostat -> client). |
da6dc3e
to
fed0d01
Compare
Rebased, the only conflict was just imports in the MainModule. |
fed0d01
to
1ab03b5
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.
Looks good! Will wait until @hareetd has had a chance to review this thoroughly
src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandler.java
Show resolved
Hide resolved
src/main/java/io/cryostat/net/web/http/api/v1/TargetRecordingsPostHandler.java
Show resolved
Hide resolved
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.
Looks good!
With this change, adding a new rule definition causes that rule to become immediately activated for any matching targets that are already known. Prior to this change adding a new rule definition would only affect targets that (re)appear after the rule was added.
Example: