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

feat(rules): enable/disable automated rules #1084

Merged
merged 7 commits into from
Oct 3, 2022

Conversation

lkonno
Copy link
Contributor

@lkonno lkonno commented Sep 27, 2022

Fixes #773

http PATCH :8181/api/v2/rules/demo?enabled=false

When a rule created disabled is enabled, the rule is activated and it starts recording.

@andrewazores andrewazores added the feat New feature or request label Sep 28, 2022
@andrewazores
Copy link
Member

Looks mostly good from a reading. I haven't tested it out yet but judging by the test changes I think it would work as expected.

One change I would like to see is to use the request body for the PATCH rather than using a query parameter. This is a little more conventional and also matches how we stop or archive active recordings:

Here we get the request body as a simple String, which is either "save" (copy to archives) or "stop" (RUNNING active recording becomes STOPPED).

So rather than PATCH /api/v2/rules/:ruleName?enabled=state it would be simply PATCH /api/v2//rules/:ruleName with a request body content. I think the natural thing to do for the body content is to expect it as a JSON object like { "enabled": false }. After parsing the JSON the rest of the request would be handled the same way you're currently doing it.

Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

It might be worth adding an integration test here. I think it can be pretty simple:

- create a rule definition with enabled=false
- start an external target container that matches the rule's matchExpression
- query the list of recordings on that target and assert that the list is empty
- patch the rule to enable it
- sleep a few seconds
- query the recording list again and assert that it has one recording with the expected name

@lkonno lkonno marked this pull request as draft September 29, 2022 13:46
@lkonno lkonno force-pushed the enable-disable-automated-rules branch from 4964b26 to 62043c6 Compare September 30, 2022 21:13
@lkonno lkonno marked this pull request as ready for review September 30, 2022 21:15
@lkonno
Copy link
Contributor Author

lkonno commented Sep 30, 2022

It might be worth adding an integration test here. I think it can be pretty simple:

- create a rule definition with enabled=false
- start an external target container that matches the rule's matchExpression
- query the list of recordings on that target and assert that the list is empty
- patch the rule to enable it
- sleep a few seconds
- query the recording list again and assert that it has one recording with the expected name

Created a new test in the AutoRulesIT.

@lkonno lkonno force-pushed the enable-disable-automated-rules branch from 82f5467 to df52e59 Compare October 3, 2022 18:56
Copy link
Member

@andrewazores andrewazores left a comment

Choose a reason for hiding this comment

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

Looks good, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feat New feature or request needs-documentation
Projects
No open projects
Status: Done
Development

Successfully merging this pull request may close these issues.

[Story] Enable/disable Automated Rules
3 participants