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

Implement override triggers. #825

Merged
merged 23 commits into from
Sep 26, 2023
Merged

Implement override triggers. #825

merged 23 commits into from
Sep 26, 2023

Conversation

floitsch
Copy link
Member

No description provided.

@floitsch floitsch requested a review from kasperl September 20, 2023 13:24
Copy link
Contributor

@kasperl kasperl 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 to me.

@@ -240,6 +266,23 @@ class ContainerJob extends Job:
stringify -> string:
return "container:$name"

deep-sleep-state -> any:
job-state := super
Copy link
Contributor

Choose a reason for hiding this comment

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

job-state <> deep-sleep-state

Copy link
Member Author

Choose a reason for hiding this comment

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

changed to state.

deep-sleep-state -> any:
job-state := super
if not override-triggers_: return job-state
return {
Copy link
Contributor

Choose a reason for hiding this comment

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

This looks clean, but we're only storing in RTC memory so we don't have to worry about upgradability. Maybe just stick to lists that are much shorter when encoded?

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -24,3 +24,7 @@ class ContainerCurrent implements artemis.Container:
triggers -> List:
encoded-triggers := client_.container-current-triggers
return encoded-triggers.map: artemis.Trigger.decode it

set-next-start-triggers triggers/List -> none:
Copy link
Contributor

Choose a reason for hiding this comment

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

triggers can be null

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

kind := trigger.kind
if kind == Trigger.KIND-BOOT:
trigger-boot = true
if kind == Trigger.KIND-INSTALL:
Copy link
Contributor

Choose a reason for hiding this comment

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

else if here and elsewhere.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -219,6 +244,7 @@ class ContainerJob extends Job:
is-background_/bool := false

triggers_/Triggers := Triggers
override_triggers_/Triggers? := null
Copy link
Contributor

Choose a reason for hiding this comment

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

I'd consider calling these triggers-override_, but if nothing else I'd kebabify.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

return triggers_.to-encoded-list
return active-triggers.to-encoded-list

active-triggers -> Triggers:
Copy link
Contributor

Choose a reason for hiding this comment

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

Active sounds a little bit like they've fired already. Maybe effective? Also, I'd consider just having a field with the effective triggers and then have the default ones hide behind that.

triggers_/Triggers
triggers-default_/Triggers := Triggers

Fewer null checks and helper methods.

Copy link
Member Author

Choose a reason for hiding this comment

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

Changed to two fields, where triggers-armed_ is usually identical to the triggers-default_ but may be overridden.

If $triggers is set to null, then the original triggers are
restored.
*/
set-next-start-triggers triggers/List? -> none
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure yet, but I feel like I want to couple this with the container restart mechanism.

artemis.Container.current.restart --triggers=[ ... ] --delay=(Duration --s=10)

Copy link
Member Author

Choose a reason for hiding this comment

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

We can add this functionality, but Consibio wants to be able to disable a pin-trigger. They might not necessarily need to restart at that point.
It's also easier if the program decides at start-up what its next triggers should be. (Or maybe just after connecting...).
If we force a restart when setting the trigger, then it's quite easy to forget it, or to accidentally not set if if the program crashes...

@@ -155,6 +165,9 @@ class ArtemisServiceProvider extends ChannelServiceProvider
container-current-triggers -> List:
unreachable // Here to satisfy the checker.

container-current-set-triggers new-triggers/List -> none:
unreachable // Here to satisfy the checker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unreachable // Here to satisfy the checker.
unreachable // Here to satisfy the checker.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

@@ -155,6 +165,9 @@ class ArtemisServiceProvider extends ChannelServiceProvider
container-current-triggers -> List:
unreachable // Here to satisfy the checker.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
unreachable // Here to satisfy the checker.
unreachable // Here to satisfy the checker.

Copy link
Member Author

Choose a reason for hiding this comment

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

done.

Base automatically changed from floitsch/triggers.50.deep-sleep-state2 to main September 26, 2023 08:45
Copy link
Contributor

@kasperl kasperl left a comment

Choose a reason for hiding this comment

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

Still looks good.

@floitsch floitsch merged commit e4bff59 into main Sep 26, 2023
@floitsch floitsch deleted the floitsch/triggers.60.override branch September 26, 2023 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants