docker-archive / deploykit

A toolkit for creating and managing declarative, self-healing infrastructure.
Apache License 2.0
2.25k stars 262 forks source link

Make `Group.CommitGroup()` plural #304

Closed wfarner closed 7 years ago

wfarner commented 7 years ago

Change this:

type Plugin interface {
    CommitGroup(grp Spec, pretend bool) (string, error)
}

to

type Plugin interface {
    CommitGroups(groups []Spec, pretend bool) (string, error)
}

This will make it natural to expose tools that interact with multiple groups.

wfarner commented 7 years ago

@chungers any opinions on whether this should change the behavior of infrakit group commit to be declarative?

i.e. currently, if i run commit group-a then commit group-b, i am left with 2 groups.

The yet-unmentioned detail with this change is that CommitGroups becomes declarative, and the above sequence could be changed as well such that only group-b remains.

(Note: this seems to diminish the need for the existing group Destroy action, as it is implicit behavior of Commit to destroy any existing groups that aren't in the commit.)

chungers commented 7 years ago

I've been going back and forth on this one. On one hand, we need to support multiple groups, one the other, I am starting to think we should just leave the group api as-is to keep the api at this level simple and avoid the issues you've described. Right now the group plugin api makes good sense on a per-group basis. Making commit accept a list of groups also makes free look strange -- per the point you just raised about destroy.

I think it's better to let another layer do the proper orchestration of multiple groups across different drivers and different resource types. This layer (think of it as the CFN-like orchestrator) will have a better idea of the entire landscape of resources and groups that depend on them. It can better compute the changes and can make commit / destroy decisions.

Because this higher global orchestration layer has to work with groups and other resource types / plugin types, it can come out after v0.1.0 along with other resource/plugin types. What do you think?

wfarner commented 7 years ago

Agreed, closing for now.