-
-
Notifications
You must be signed in to change notification settings - Fork 56
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
automations #1343
automations #1343
Conversation
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThis pull request introduces a comprehensive refactoring of the application's integration and automation capabilities. The changes replace the existing OSC and HTTP integration services with a new, more flexible automation system. The new architecture allows for creating and managing automation blueprints and rules that can trigger actions based on timer lifecycle events. The modifications span both client and server-side code, introducing new components for managing automation settings, creating blueprints, and handling output dispatching. Changes
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
e6ee477
to
cd34149
Compare
a20ed53
to
1264147
Compare
6409c38
to
717a1ce
Compare
399fe0e
to
e3246f5
Compare
|
||
export const automationPlaceholderSettings: AutomationSettings = { | ||
enabledAutomations: false, | ||
enabledOscIn: false, |
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.
@alex-Arc i am wondering if the OSC settings should be in the app settings instead of here
What do you think?
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.
yes
at least it should be separate from the automations
const fieldValue = state[field]; | ||
|
||
// TODO: if value is empty string, the user could be meaning to check if the value does not exist | ||
switch (operator) { |
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.
@alex-Arc i have been wondering if we could come up with a few examples and fine tune the operators?
* Expose possibility to send a message using OSC protocol | ||
*/ | ||
export function emitOSC(output: OSCOutput, state: RuntimeState) { | ||
const message = preparePayload(output, state); |
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.
Skip sending in cloud
a5663da
to
97d34bf
Compare
@alex-Arc , I believe this is ready to merge, would you mind sanity checking? |
97d34bf
to
f9b9e64
Compare
@@ -26,8 +26,8 @@ describe('simple tests for regex', () => { | |||
}); | |||
|
|||
test('startsWithHttp', () => { | |||
const right = ['http://test']; | |||
const wrong = ['https://test', 'testing', '123.0.1']; | |||
const right = ['https://test', 'http://test']; |
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.
Did you get https working?
or did it always work we just did not allow it
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.
i have been testing with ontime cloud and it seems fine. can you verify in one of your containers?
@@ -283,7 +257,6 @@ export const shutdown = async (exitCode = 0) => { | |||
|
|||
expressServer?.close(); | |||
runtimeService.shutdown(); |
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.
do we not still need the osc shutdown here?
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.
yes we do, good catch
f9b9e64
to
b732178
Compare
merge with automation service
rename automations > triggers rename blueprints > automations chore: add link to documentation
4af6b14
to
e4a4f78
Compare
e4a4f78
to
c4e2c81
Compare
This is an attempt at re-writing the integrations service
For simplicity, I am renaming this to automation, this means that integrations becomes an internal name used for data into ontime
There are also some dependency upgrades which make the PR larger
The code parsing logic is the same as before, so hopefully there are not that many core changes to the outputs, however, the filtering logic is completely new
I am also proposing a new structure to our backend, it is annoying that I only did it in automations and - if we decide to go forwards with this - I will create a follow up PR to apply everywhere
dao
fileautomation
directory, contains all logic related to that domain conceptcontroller
androuter
files, but didnt see an obvious improvement. in a way, it is nice to be able to read all the available routes in a small file without worrying about what they doIn this PR
create migration code for old subscriptions(decided to migrate OSC data and leave the rest)TODO