-
Notifications
You must be signed in to change notification settings - Fork 22
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(containers): run ready actions when the deployment is ready #731
feat(containers): run ready actions when the deployment is ready #731
Conversation
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.
lgtm
2795152
to
f81c876
Compare
f81c876
to
bccdbc0
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.
Overall approval, just a small comment on run_ready_actions.ex
Enum.reduce_while(deployment.ready_actions, {:ok, deployment}, fn action, _acc -> | ||
case Containers.run_ready_action(action) do | ||
{:ok, _ready_action} -> {:cont, {:ok, deployment}} | ||
{:error, reason} -> {:halt, {:error, reason}} | ||
end | ||
end) |
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.
Maybe here we don't actually want to halt if one ready action fails, I think it would be better to just run the ready action and let them crash if something happens
Enum.reduce_while(deployment.ready_actions, {:ok, deployment}, fn action, _acc -> | |
case Containers.run_ready_action(action) do | |
{:ok, _ready_action} -> {:cont, {:ok, deployment}} | |
{:error, reason} -> {:halt, {:error, reason}} | |
end | |
end) | |
_ready_actions = Enum.map(deployment.ready_actions, &Containers.run_ready_action/1)) | |
{:ok, deployment} |
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.
Hard to say now because we don't have any information on what we want to do with multiple ready actions for the same deployment.
It's a problem we will encounter once the situation arises
Signed-off-by: Francesco Noacco <[email protected]>
bccdbc0
to
6c12827
Compare
50c3ce5
into
edgehog-device-manager:feature/application-management
drafted because it depends on #724 and 724 is not ready, but 2795152 is ready for review