Action runner cleanup #3

Merged
merged 5 commits into from Mar 10, 2014

Conversation

Projects
None yet
2 participants
@vito
Contributor

vito commented Mar 9, 2014

This ties up the rest of the loose ends with the big ol' Action Runner refactor.

Mainly:

  • re-decouples the download/upload/run actions from RunOnecs
  • moves these actions out of the runoncehandler directory
  • improves the RunOnceHandler's test so it exercises more than just order and type of actions being executed. it can now actually catch bugs in how the actions are constructed (e.g. execute action's actions being performed without a container handle because they were constructed too early)

vito added some commits Mar 8, 2014

relocate generic actions out of execute action
These are intended to not be coupled to RunOnces. Decoupling will be done in
a later commit.
decouple generic actions from RunOnce
Introduce lazy action runner; given a thunk, it'll invoke it when being
Performed to generate its actions. This allows the execute action to be
created before we know the handle/etc. of the RunOnce.
+
+ BeforeEach(setupSuccessfulRuns)
+
+ It("registers, claims, creates container, starts, (executes...), completes", func() {

This comment has been minimized.

Show comment Hide comment
@onsi

onsi Mar 9, 2014

Contributor

I love this mini-integration test.. much better

@onsi

onsi Mar 9, 2014

Contributor

I love this mini-integration test.. much better

@onsi

This comment has been minimized.

Show comment Hide comment
@onsi

onsi Mar 9, 2014

Contributor

Taking a quick look, this makes a lot of sense to me - great work, Alex. The RunOnceHandler test is a lot better and the generic actions are nicely decoupled from RunOnces. The LazyActionRunner is a cool idea and I think it helps solve the decoupling problem (RunOnce vs ContainerHandle) at the right abstraction level.

We have lots of mini-packages now and I'm wondering if we can organize things a bit more to make the top-level a little tighter (this can wait till Monday obviously). Some thoughts:

  • could the log_streamer_factory just move into logstreamer? (we could have logstreamer.New and logstreamer.NewFactory and then fakelogstreamer.New and fakelogstreamer.NewFactory (which could return a factory function and the FakeLogStreamer that it will inject in..)
  • we can move extractor, linuxplugin, uploader, downloader -- all things that support the actions -- either into actions/support or actions_support.
  • we can flatten out the actions in the actions directory to just be .go files int he actions package (so: actions.NewUploadAction instead of upload_action.New) I don't think they really need to be separate packages and it'll help keep the tests snappy. But this is mostly subjective opinion
  • same with run_once_handler and all its actions. We could just have run_once_handlers/run_once_actions with a bunch of .go files

Just some seed-thoughts for discussion. I'm primarily concerned with test speed (lots of packages = lots of compiling = slightly slower suite) and with grokability (which is subjective and might just be me). Also, I can't decide if the one package = one object rule is a go pattern or a go antipattern and I think logically grouping related things (like actions) into one package makes a lot of sense (again, to me at least...)

Contributor

onsi commented Mar 9, 2014

Taking a quick look, this makes a lot of sense to me - great work, Alex. The RunOnceHandler test is a lot better and the generic actions are nicely decoupled from RunOnces. The LazyActionRunner is a cool idea and I think it helps solve the decoupling problem (RunOnce vs ContainerHandle) at the right abstraction level.

We have lots of mini-packages now and I'm wondering if we can organize things a bit more to make the top-level a little tighter (this can wait till Monday obviously). Some thoughts:

  • could the log_streamer_factory just move into logstreamer? (we could have logstreamer.New and logstreamer.NewFactory and then fakelogstreamer.New and fakelogstreamer.NewFactory (which could return a factory function and the FakeLogStreamer that it will inject in..)
  • we can move extractor, linuxplugin, uploader, downloader -- all things that support the actions -- either into actions/support or actions_support.
  • we can flatten out the actions in the actions directory to just be .go files int he actions package (so: actions.NewUploadAction instead of upload_action.New) I don't think they really need to be separate packages and it'll help keep the tests snappy. But this is mostly subjective opinion
  • same with run_once_handler and all its actions. We could just have run_once_handlers/run_once_actions with a bunch of .go files

Just some seed-thoughts for discussion. I'm primarily concerned with test speed (lots of packages = lots of compiling = slightly slower suite) and with grokability (which is subjective and might just be me). Also, I can't decide if the one package = one object rule is a go pattern or a go antipattern and I think logically grouping related things (like actions) into one package makes a lot of sense (again, to me at least...)

@vito

This comment has been minimized.

Show comment Hide comment
@vito

vito Mar 9, 2014

Contributor

thanks; fyi I expect this to be something dealt with monday, not a trivial PR like the other bash scripting one was :)

two conflicting thoughts:

  1. I don't like code organization being influenced by e.g. a compiler or other tool, instead of a human brain
  2. I tend to err on the side of "new package" without much thought

Given that most of the packages I've made are probably from the latter tendency it's probably worth circling back and seeing if the practical benefits are worth compromising the former thought, and/or if they make sense organized differently anyway. Some of the remaining packages ended up being just functions (like log_streamer_factory).

Contributor

vito commented Mar 9, 2014

thanks; fyi I expect this to be something dealt with monday, not a trivial PR like the other bash scripting one was :)

two conflicting thoughts:

  1. I don't like code organization being influenced by e.g. a compiler or other tool, instead of a human brain
  2. I tend to err on the side of "new package" without much thought

Given that most of the packages I've made are probably from the latter tendency it's probably worth circling back and seeing if the practical benefits are worth compromising the former thought, and/or if they make sense organized differently anyway. Some of the remaining packages ended up being just functions (like log_streamer_factory).

vito added a commit that referenced this pull request Mar 10, 2014

@vito vito merged commit 2918ce9 into master Mar 10, 2014

1 check passed

default The Travis CI build passed
Details

@vito vito deleted the action-runner-cleanup branch Jul 27, 2014

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment