Support tweaking the operation before running it #112
Conversation
carolynvs
requested review from
radu-matei,
glyn,
silvin-lubecki and
jeremyrickard
There was a problem hiding this comment.
I proposed a couple of small refactorings, but I'm also interested to know whether duffle could use this mechanism to mount relocation mapping files instead of using its current approach of wrapping the driver. A draft duffle PR utilising this PR would be great (and I'm happy to help code it once this PR is ready to merge).
action/install.go
Outdated
| @@ -25,6 +29,10 @@ func (i *Install) Run(c *claim.Claim, creds credentials.Set, w io.Writer) error | |||
| return err | |||
| } | |||
|
|
|||
| if i.OperationConfig != nil { | |||
There was a problem hiding this comment.
What about adding a variadic parameter to the Run function signature instead of storing it into the structs?
func (i *Install) Run(c *claim.Claim, creds credentials.Set, w io.Writer, ops OperationConfig...) error
Something like this? I guess it is a more functional way to apply custom configs?
WDYT?
There was a problem hiding this comment.
I originally made the change that way. 😀 It would be a breaking change since it alters the method signature. I wasn't sure if that would be a welcome change(?) so I went with this instead for an initial proposal.
It looked like this:
func (i *Install) Run(c *claim.Claim, creds credentials.Set, ops OperationConfig...)
Setting the writer on the op was something that the caller managed via the ops parameter.
There was a problem hiding this comment.
Ok I was wondering about that too, but as we didn't hit the 1.0 yet, should we "care" about about breaking changes? I think we should focus on the API design instead 😄
There was a problem hiding this comment.
Here's what it would look like with @silvin-lubecki 's suggested change.
type OperationConfigFunc func(op *driver.Operation) error
type OperationConfigs []OperationConfigFunc
// ApplyConfig safely applies the configuration function to the operation, if defined,
// and returns any error.
func (cfgs OperationConfigs) ApplyConfig(op *driver.Operation) error {
var err error
for _, cfg := range cfgs {
err = cfg(op)
if err != nil {
return err
}
}
return nil
}
// Run performs an installation and updates the Claim accordingly
func (i *Install) Run(c *claim.Claim, creds credentials.Set, w io.Writer, opCfgs ...OperationConfigFunc) error {
// ...
err = OperationConfigs(opCfgs).ApplyConfig(op)
if err != nil {
return err
}
I'm leaving the writer alone for now just to show what the overall change looks like. It could made into a config func that everyone would have to pass in (more disruptive), or we could keep it as is.
There was a problem hiding this comment.
@glyn What do you think?
|
I just dug up what duffle is doing (below) immediately before run and I think this would work for duffle as well. Seems to me that duffle could add an config action with this function's logic and not need the wrapper. |
|
@glyn Ok, just pushed up another commit with your suggested changes. |
action/configurable_action.go
Outdated
| type ConfigurableAction struct { | ||
| // OperationConfig is an optional handler that applies additional | ||
| // configuration to an operation before it is executed. | ||
| OperationConfig OperationConfigFunc |
There was a problem hiding this comment.
I would have prefer a slice here, so user can have multiple unitary config modifiers. I guess it would help factorizing between different actions. WDYT?
| }, | ||
| Error: nil, | ||
| } | ||
| inst := &RunCustom{Driver: d, Action: "test"} |
There was a problem hiding this comment.
I think we should factorize those tests, as they are exactly the same for all the actions (good hint is that this action is still called inst 😇 ).
There was a problem hiding this comment.
LGTM, thanks for making those changes. I'm going to hold off seeing how this can help in duffle given that @silvin-lubecki is asking for more changes.
This will allow a tool to apply additional configuration, such as injecting additional files or environment variables, before the operation is run. In our case, it will allow Porter to continue to implement dependencies without having to make extensive changes to cnab-go because it can inject the changes it requires through this configuration function instead of modifying the cnab-go code directly.
carolynvs
force-pushed
the
op-config-func
branch
from
dd2cd70 to
e65856c
Compare
This switches to applying multiple configs via the Run interface. It includes applying the writer as well since it falls into the scope of the new OperationalConfig.
|
@silvin-lubecki @glyn I've updated the PR with Silvin's requested change. If you could take another look, I'm hoping to get some form of this PR (either this or the former) merged before the cutoff. Thanks! |
| @@ -9,7 +9,7 @@ import ( | |||
| "github.com/stretchr/testify/assert" | |||
| "github.com/stretchr/testify/require" | |||
|
|
|||
| "gopkg.in/yaml.v2" | |||
| yaml "gopkg.in/yaml.v2" | |||
There was a problem hiding this comment.
nit: I think this alias is no-op, go already uses the part before the first dot as import name.
There was a problem hiding this comment.
Yeah I'm aware it's a noop. It's an artifact of using GoLand for an IDE and for the life of me I can't figure out how to stop it from introducing those. When I remove the alias, it adds it back. 🤷♀ Running goimports (which I do on file save) doesn't clean those up either.
There was a problem hiding this comment.
I just asked the twitterverse for help. https://twitter.com/carolynvs/status/1169993866503757827
There was a problem hiding this comment.
Thank you @carolynvs 🤗 I only have few nit/comments, but non blocking, could be made in a follow-up 👍
This will allow a tool to apply additional configuration, such as injecting additional files or environment variables, before the operation is run.
In our case, it will allow Porter to continue to implement dependencies without having to make extensive changes to cnab-go because it can inject the changes it requires through this configuration function instead of modifying the cnab-go code directly.