split commands.go into multiple files #118
Conversation
app/main.go
Outdated
| @@ -50,14 +50,17 @@ type HTTPTransportConfig struct { | |||
| KeepAlive time.Duration | |||
| } | |||
|
|
|||
| // SHA256Sums alias for the sha256 sums | |||
| type SHA256Sums []string | |||
There was a problem hiding this comment.
made this because I think I need to kong.Bind it too
There was a problem hiding this comment.
because https://github.com/cashapp/hermit/pull/118/files#diff-54f9129c1a376afcd83728c59ff47e91d9de0b2672d4312179bf28e75846d9d5R18 . and because reflection, this only comes up at runtime. would be good to have some integration test that shells out and actually runs the commands and checks expected results
There was a problem hiding this comment.
Ah, specifically because otherwise you would have an import cycle. I see.
would be good to have some integration test that shells out and actually runs the commands and checks expected results
There are integration tests in the it sub-directory that should exercise this, among other things.
app/main.go
Outdated
| @@ -50,14 +50,17 @@ type HTTPTransportConfig struct { | |||
| KeepAlive time.Duration | |||
| } | |||
|
|
|||
| // SHA256Sums alias for the sha256 sums | |||
| type SHA256Sums []string | |||
|
Sorry for the delay, but on consideration I think I'd prefer it if the commands were still contained in the
Would you mind pulling these back in, but in separate files as you have here? |
app/main.go
Outdated
| @@ -50,14 +50,17 @@ type HTTPTransportConfig struct { | |||
| KeepAlive time.Duration | |||
| } | |||
|
|
|||
| // SHA256Sums alias for the sha256 sums | |||
| type SHA256Sums []string | |||
There was a problem hiding this comment.
Ah, specifically because otherwise you would have an import cycle. I see.
would be good to have some integration test that shells out and actually runs the commands and checks expected results
There are integration tests in the it sub-directory that should exercise this, among other things.
yea, when I made the commands package I was hoping it wouldn't have too many side effects. |
I'd prefer to keep it in |
|
Thanks for doing this BTW and sorry for the run-around. |
yea, I can do that. why isn't |
It's deliberately public to allow Square to have a custom Hermit binary. |
|
@alecthomas I couldn't figure out how to run |
|
CI got wedged because ubuntu-16.04 was deprecated (which took about a day to determine). I've pushed a fix to master but you'll have to rebase and push for this to get picked up. |
mihai-chiorean
force-pushed
the
break-commands
branch
from
71bcc48
to
ac2678a
Compare
|
The integration tests should be runnable by executing any of the |
|
mihai-chiorean
force-pushed
the
break-commands
branch
from
1c2b2f9
to
745bfb8
Compare
|
@alecthomas I finally got around to moving this on my laptop where I was able to run the integration tests and make the fixes. mind taking another pass at it? |
|
LGTM! |
|
Thanks for persisting. |
commands.gowas looking increasingly large. I broke it into separate files and it's own package to keep things tidy.Note that some small commands (noop, version) I left behind just because they were small and more... operational).
Also a caveat is that
app/commands/does not have access toConfig, so I had to work around that by binding the http client and the env vars. I'm open to better ideas there..Added a little something to the makefile, in case it eventually gets bigger. Looks like this:
)