-
Notifications
You must be signed in to change notification settings - Fork 10
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
Feature/93 expand executor interface #94
Feature/93 expand executor interface #94
Conversation
7c0300c
to
170a612
Compare
internal/decomposer/decomposer.go
Outdated
case "action": | ||
return decomposer.ExecuteActionStep(step, variables) | ||
case cacao.StepTypeAction: | ||
details := action.StepDetails{Step: step, Targets: decomposer.playbook.TargetDefinitions, Auth: decomposer.playbook.AuthenticationInfoDefinitions, Agent: decomposer.playbook.AgentDefinitions[step.Agent], Variables: variables} |
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 personally find slightly confusing the naming of the struct fields here, because some are step-specific and some are not.
In StepDetails, we are inserting all targets definitions and all authinfo. I would then name the struct fields something more like PlaybookTargets, PlaybookAuthInfoDefinitions, and StepAgent.
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 playbook metadata is better?
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.
Not sure what you mean. You mean instead of StepDetails? The role of the "details" variable is still to provide step details and related step context right? And also there's already the "metadata" var being used.
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.
can you also make this line shorter?
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.
Had a comment on the StepDetails struct. But overall looks nice and tidier than before.
internal/decomposer/decomposer.go
Outdated
case "action": | ||
return decomposer.ExecuteActionStep(step, variables) | ||
case cacao.StepTypeAction: | ||
details := action.StepDetails{Step: step, Targets: decomposer.playbook.TargetDefinitions, Auth: decomposer.playbook.AuthenticationInfoDefinitions, Agent: decomposer.playbook.AgentDefinitions[step.Agent], Variables: variables} |
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.
can you also make this line shorter?
No description provided.