host-ctr: refactoring #1235
host-ctr: refactoring #1235
Conversation
etungsten
force-pushed
the
host-ctr-refactor
branch
from
9883a4f to
210f7d4
Compare
| } | ||
|
|
||
| func _main() int { | ||
| // Parse command-line arguments | ||
| func _main() error { |
There was a problem hiding this comment.
Instead of returning the result of having called the command, the cli.App type should be returned for direct use in testing and have that cli.App.Run called from the real main. This gives a little more control to testing entrypoints where Run can optionally be called.
If that were changed, I'd rename this to func App() cli.App (App because it stands out a bit more than app, not because it needs to exported and because I personally like it :P)
| var ( | ||
| containerID string | ||
| source string | ||
| containerdSocket string | ||
| namespace string | ||
| superpowered bool | ||
| pullImageOnly bool | ||
| ) |
There was a problem hiding this comment.
I want to say change this to not use predeclared vars, but I can't come up with a good reason right now to make it otherwise (using cli.Context.String() and friends instead). I'm in favor of leaving this as-is for now (Sam and I have debated this before ourselves, I'm ambivalent).
There was a problem hiding this comment.
I like the predeclared vars.
There was a problem hiding this comment.
I like the predeclared vars.
I know, but it's less clear in these circumstances because the two subcommands are each using them. There's not a lot of them and so the scope remains clear for now - I would push on this if there were more at play. For now, I think its reasonable and good to move forward with.
| } | ||
|
|
||
| img, err := pullImage(ctx, ref, client) | ||
| // Check if the target container already exists. If it does, take over the helm to manage it. |
There was a problem hiding this comment.
Can we separate the functional changes from the refactoring changes?
There was a problem hiding this comment.
Oh oops. I totally forgot that this came along with my cherry pick.
Refactors `host-ctr`. Organizes functionality into subcommands.
etungsten
force-pushed
the
host-ctr-refactor
branch
from
210f7d4 to
553e770
Compare
| var ( | ||
| containerID string | ||
| source string | ||
| containerdSocket string | ||
| namespace string | ||
| superpowered bool | ||
| pullImageOnly bool | ||
| ) |
There was a problem hiding this comment.
I like the predeclared vars.
I know, but it's less clear in these circumstances because the two subcommands are each using them. There's not a lot of them and so the scope remains clear for now - I would push on this if there were more at play. For now, I think its reasonable and good to move forward with.
Issue number:
N/A
Description of changes:
Testing done:
Build image and launched several instances and all host containers came up fine. kubelet running fine, meaning pause container got pulled successfully.
Terms of contribution:
By submitting this pull request, I agree that this contribution is dual-licensed under the terms of both the Apache License, version 2.0, and the MIT license.