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

LocalCommand state management #36

Merged
merged 5 commits into from
Aug 21, 2024
Merged

Conversation

edouardpoitras
Copy link
Owner

@edouardpoitras edouardpoitras commented Aug 14, 2024

Each LocalCommand now manages it's own state.
This makes it easier to have multiple addons interacting with each other.
Also makes the implementation of those addons simpler.

Added the Delay component which is used to delay command execution.
It also works well with the Retry component (applies the delay for every retry).

Future work includes adding a chaining mechanism (run multiple commands) and adding more delay types.

Apologies for the large PR. Will keep it short for future enhancements.

@edouardpoitras edouardpoitras force-pushed the process-state-management branch from 3668fa9 to 9bedd35 Compare August 14, 2024 20:43
@edouardpoitras edouardpoitras force-pushed the process-state-management branch 2 times, most recently from 8be8b1f to 5c2f520 Compare August 14, 2024 21:01
@edouardpoitras edouardpoitras force-pushed the process-state-management branch from 5c2f520 to 2affa98 Compare August 14, 2024 21:01
@edouardpoitras edouardpoitras linked an issue Aug 14, 2024 that may be closed by this pull request
@edouardpoitras edouardpoitras marked this pull request as ready for review August 14, 2024 21:04
Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Interesting approach seems to make the state management easier.

I'm not sure if now it's maybe a bit harder to implement an external ad-on that's outside of this crate?
And maybe the internal systems are getting a bit large as they consider multiple ad-ons at once.

On the other hand, some ad-ons have to know about each other and maybe adding external ones isn't even an important use-case...

src/local_command.rs Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
@edouardpoitras
Copy link
Owner Author

edouardpoitras commented Aug 20, 2024

Interesting approach seems to make the state management easier.

I'm not sure if now it's maybe a bit harder to implement an external ad-on that's outside of this crate? And maybe the internal systems are getting a bit large as they consider multiple ad-ons at once.

On the other hand, some ad-ons have to know about each other and maybe adding external ones isn't even an important use-case...

It's entirely possible I've made other aspects of the implementation or extension capabilities harder out of my selfish desire to get the Delay and Chain components in the code. Admittedly I haven't been considering external ad-ons at all at this point. I assumed we would need to expose more of the data types/fields for that to be possible (in future version - maybe upon issue/request from the community).

I was hoping this would make the internal ad-ons simpler. You're right in the sense that the Cleanup ad-on is now getting a bit messy as it's trying to cleanup all these other ad-on components. On that front I was hoping to figure out a way to make the Cleanup component take a generic parameter. Then you could add multiple Cleanup<T> components and control exactly which components you want to cleanup upon process completion. This would solve the coupling issue as it wouldn't need any other ad-on references. Will maybe give that a shot next.

Otherwise, the only other component that I haven't yet figured out how to decouple completely, is the other PR on the Chain component. Chain + Cleanup is a problem because the cleanup tries to execute after every LocalCommand in the chain completes. Maybe there's a better way to do the Chain so that they are all entirely new Entities for each chain component... remains to be investigated.

I feel comfortable with the current short-comings of these changes and am open to iterating/reverting some of the decisions made here in the future if necessary.

Copy link
Collaborator

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Makes sense, let's not try to future-proof for scenarios that might not be that relevant and instead improve the status-quo :)

@edouardpoitras edouardpoitras merged commit 4bfa855 into main Aug 21, 2024
3 checks passed
@edouardpoitras edouardpoitras deleted the process-state-management branch August 23, 2024 22:57
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.

Add ability to delay command execution
2 participants