Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Refactor linux manual provider code. #6414
Conversation
| @@ -24,6 +21,8 @@ import ( | ||
| "github.com/juju/juju/state" | ||
| "github.com/juju/juju/state/stateenvirons" | ||
| jujuversion "github.com/juju/juju/version" | ||
| + "github.com/juju/loggo" |
| @@ -43,6 +35,13 @@ import ( | ||
| "github.com/juju/juju/testing/factory" | ||
| jujuversion "github.com/juju/juju/version" | ||
| "github.com/juju/juju/worker" | ||
| + "github.com/juju/loggo" |
axw
Oct 10, 2016
Member
Please move these back. I won't keep repeating; please check the other files yourself.
| + return "", host | ||
| +} | ||
| + | ||
| +func (c *addCommand) scopeRun(client AddMachineAPI, config *config.Config, ctx *cmd.Context) error { |
| +} | ||
| + | ||
| +func (c *addCommand) scopeRun(client AddMachineAPI, config *config.Config, ctx *cmd.Context) error { | ||
| + logger.Infof("manual provisioning") |
| + } | ||
| + | ||
| + user, host := splitUserHost(c.Placement.Directive) | ||
| + args := manualcommon.ProvisionMachineArgs{ |
axw
Oct 10, 2016
Member
ISTM this should be part of the package "manual", which is the public-facing API. manualcommon should include bits that each of the manual implementations uses internally.
hoenirvili
Oct 10, 2016
Contributor
@axw reasons I moved the ProvisionMachineArgs into the common package:
- If the type is situated in manual root pkg and the type is used in sub packages of the manual pkg, like, for example in the linux pkg, I'm getting this error:
can't load package: import cycle not allowed
package github.com/juju/juju/environs/manual
imports github.com/juju/juju/environs/manual/linux
imports github.com/juju/juju/environs/manual
import cycle not allowed
package github.com/juju/juju/environs/manual
imports github.com/juju/juju/environs/manual/linux
imports github.com/juju/juju/environs/manual
- Common is a subpackage of the manual pkg, so In my opinion, I think it's ok to justify that the "common" name refers to a common type used in the manual pkg and its subpackages.
- The name is revealing that it's just a type for passing arguments to the api caller.
axw
Oct 11, 2016
•
Edited 1 time
-
axw
Oct 11, 2016
Member
-
Oct 11, 2016
Right, but you don't have to expose manualcommon like that. One option would be to have package manual define a ProvisionMachineArgs struct, and then define it again inside package manual/common. The implementations would point at the manual/common one.
I think what I'd prefer, though, is to not have the manual package depend on the implementations at all. Instead, have something like:
manual/
sshprovisioner/
winrmprovisioner/
...
with the *provisioner packages depending on environs/manual for the common interface. Inside environs/manual, you would have an interface that each of those provisioners implements, something like:
type ManualProvisioner interface {
ProvisionMachine(ProvisionMachineParams) (machineId string, err error)
}
And then you would have a registry of ManualProvisioners that you can get one by passing in a placement directive. AFAICR, we only need that registry at the CLI, so it could be defined there. Preferably not a global.
hoenirvili
Oct 12, 2016
Contributor
@axw thanks for taking time from you holiday to enlighten me, I really appreciate. You have touched some valid points in this review but I don't think naming sshprovisioner/, winrmprovisioner/ is good, because in a couple of months windows will support ssh and linux like systems can in the future have the option to make powershell as the default shell. The ssh code already is very bashy syntax specific.
Anyway I will try to modify the current code to match your specification about the common interface and the dependency around packages.
| + }, | ||
| + } | ||
| + | ||
| + machineId, err := manualProvisioner(args, c.Placement.Scope) |
axw
Oct 10, 2016
•
Edited 1 time
-
axw
Oct 10, 2016
Member
-
Oct 10, 2016
Passing "scope" around on its own feels a bit awkward to me. It's not something we talk about in isolation, it really only makes sense in the context of a placement directive.
I think it might be a bit neater if you passed c.Placement into the manual package, either into manual.ProvisionMachine, or into a different function that returns a ssh- or winrm- specific provisioner, along with args based on the directive.
| +const ManualInstancePrefix = "manual:" | ||
| + | ||
| +type ProvisionMachineArgs struct { | ||
| + // Host is the SSH host: [user@]host |
| +type ProvisionMachineArgs struct { | ||
| + // Host is the SSH host: [user@]host | ||
| + Host string | ||
| + // User is the SSH/WINRM user [user@]host |
| +) | ||
| + | ||
| +// Script returns a valid ScriptProvisioner that will be used to produce the script. | ||
| +func Script(icfg *instancecfg.InstanceConfig) (common.ScriptProvisioner, error) { |
axw
Oct 10, 2016
Member
the name implies it would return a script, but it does not. how about NewScriptProvisioner?
|
So @axw it's +1 now? |
axw
requested changes
Oct 11, 2016
@hoenirvili Sorry, I'm actually on vacation now until 21st October. Please ping someone in #juju-dev if you don't get a proper review soon. I've left a few more thoughts, but this is not a complete review.
Please keep a separation between the OS and method of provisioning. The provisioners care primarily about the method: i.e. using SSH or WinRM. Using WinRM, obviously the OS is Windows; but that is not necessarily the case for SSH. It is right now, but that may change.
| @@ -64,7 +65,7 @@ Examples: | ||
| juju add-machine lxd -n 2 (starts 2 new machines with an lxd container) | ||
| juju add-machine lxd:4 (starts a new lxd container on machine 4) | ||
| juju add-machine --constraints mem=8G (starts a machine with at least 8GB RAM) | ||
| - juju add-machine ssh:user@10.10.0.3 (manually provisions a machine with ssh) | ||
| + juju add-machine ssh:user@10.10.0.3 (manually provisions a linux machine with ssh) |
axw
Oct 11, 2016
•
Edited 1 time
-
axw
Oct 11, 2016
Member
-
Oct 11, 2016
We may end up adding support for other *nixes, so I think this should be left as it was.
| + } | ||
| + | ||
| + user, host := splitUserHost(c.Placement.Directive) | ||
| + args := manualcommon.ProvisionMachineArgs{ |
axw
Oct 10, 2016
Member
ISTM this should be part of the package "manual", which is the public-facing API. manualcommon should include bits that each of the manual implementations uses internally.
hoenirvili
Oct 10, 2016
Contributor
@axw reasons I moved the ProvisionMachineArgs into the common package:
- If the type is situated in manual root pkg and the type is used in sub packages of the manual pkg, like, for example in the linux pkg, I'm getting this error:
can't load package: import cycle not allowed
package github.com/juju/juju/environs/manual
imports github.com/juju/juju/environs/manual/linux
imports github.com/juju/juju/environs/manual
import cycle not allowed
package github.com/juju/juju/environs/manual
imports github.com/juju/juju/environs/manual/linux
imports github.com/juju/juju/environs/manual
- Common is a subpackage of the manual pkg, so In my opinion, I think it's ok to justify that the "common" name refers to a common type used in the manual pkg and its subpackages.
- The name is revealing that it's just a type for passing arguments to the api caller.
axw
Oct 11, 2016
•
Edited 1 time
-
axw
Oct 11, 2016
Member
-
Oct 11, 2016
Right, but you don't have to expose manualcommon like that. One option would be to have package manual define a ProvisionMachineArgs struct, and then define it again inside package manual/common. The implementations would point at the manual/common one.
I think what I'd prefer, though, is to not have the manual package depend on the implementations at all. Instead, have something like:
manual/
sshprovisioner/
winrmprovisioner/
...
with the *provisioner packages depending on environs/manual for the common interface. Inside environs/manual, you would have an interface that each of those provisioners implements, something like:
type ManualProvisioner interface {
ProvisionMachine(ProvisionMachineParams) (machineId string, err error)
}
And then you would have a registry of ManualProvisioners that you can get one by passing in a placement directive. AFAICR, we only need that registry at the CLI, so it could be defined there. Preferably not a global.
hoenirvili
Oct 12, 2016
Contributor
@axw thanks for taking time from you holiday to enlighten me, I really appreciate. You have touched some valid points in this review but I don't think naming sshprovisioner/, winrmprovisioner/ is good, because in a couple of months windows will support ssh and linux like systems can in the future have the option to make powershell as the default shell. The ssh code already is very bashy syntax specific.
Anyway I will try to modify the current code to match your specification about the common interface and the dependency around packages.
| @@ -310,7 +302,7 @@ func (c *addCommand) Run(ctx *cmd.Context) error { | ||
| return errors.Trace(err) | ||
| } | ||
| - errs := []error{} | ||
| + errs := make([]error, 0, 10) // avoid extra allocations |
axw
Oct 11, 2016
•
Edited 1 time
-
axw
Oct 11, 2016
Member
-
Oct 11, 2016
The normal case is for there to be no errors at all. If anything, I think you should change this to var errs []error.
| + | ||
| +// ScriptProvisioner used to determine the script that will be used to | ||
| +// manual provision a machine linux or windows machine. | ||
| +type ScriptProvisioner interface { |
axw
Oct 11, 2016
Member
I'm curious why this is needed actually. The existing ProvisioningScript function wasn't particularly Linux-specific, so I wonder why we need a separate one for Windows?
hoenirvili
Oct 11, 2016
Contributor
If you look closely @axw in manual/linux/linux.go:276 in this function is the linux(aka sshprovisioner script generation).For a windows provisioning standpoint we don't need unsupported things enabled.
cloudcfg.SetSystemUpdate(s.icfg.EnableOSRefreshUpdate)
cloudcfg.SetSystemUpgrade(s.icfg.EnableOSUpgrade)or wrapper functions around bash scripts like
fmt.Fprintf(&buf, "rm -f %s\n", utils.ShQuote(s.icfg.CloudInitOutputLog))Yeah I know this line retuns (in a transparent way) different implementations based on different series.
udata, err := cloudconfig.NewUserdataConfig(s.icfg, cloudcfg) But when a windows specific series is chosen, we don't need all of this extra bash/ssh specific things around our script generation, instead we need our winrm (this will be contained in the future patch that I will send, as a side note) specific things. So instead of cluttering the ssh and the winrm code together ,I decoupled them in multiple implementations on the same interface type.
axw
Oct 25, 2016
Member
Fair enough. I think we can do away with the interface though, and just have a function that dispatches to others based on the InstanceConfig's series.
hoenirvili
changed the base branch from
master
to
develop
Oct 19, 2016
|
!!build!! |
kat-co
suggested changes
Oct 19, 2016
On large changes I tend to rush through comments. Please do not mistake this for curtness; thank you for the PR and for the effort in making this code more robust!
I think this needs a lot of work before it's ready to land.
| - config.EnableOSRefreshUpdate(), | ||
| - config.EnableOSUpgrade(), | ||
| - }, | ||
| + if c.Placement != nil { |
reedobrien
Oct 20, 2016
Contributor
I am sure @KatCo is asking for idiomatic error flow indentation, @hoenirvili
| + | ||
| + authKeys, err := common.ReadAuthorizedKeys(ctx, "") | ||
| + if err != nil { | ||
| + return errors.Annotate(err, "reading authorized-keys") |
| + }, | ||
| + } | ||
| + | ||
| + logger.Infof("manual provisioning") |
kat-co
Oct 19, 2016
Contributor
Is logging really necessary here? If so, please provide a more complete message.
| + logger.Infof("manual provisioning") | ||
| + machineId, err := manualProvisioner(args, c.Placement) | ||
| + if err == nil { | ||
| + ctx.Infof("created machine %v", machineId) |
| +func RecordMachineInState(client ProvisioningClientAPI, machineParams params.AddMachineParams) (machineId string, err error) { | ||
| + results, err := client.AddMachines([]params.AddMachineParams{machineParams}) | ||
| + if err != nil { | ||
| + return "", err |
| + // Currently, only one machine is added, but in future there may be several added in one call. | ||
| + machineInfo := results[0] | ||
| + if machineInfo.Error != nil { | ||
| + return "", machineInfo.Error |
kat-co
Oct 19, 2016
Contributor
I don't know the answer to this: do we want to treat the deserialized error as an error for the function?
I think probably because we want to continue down the error-handling path since the remote operation failed, but wanted to raise the question.
| + "github.com/juju/juju/environs/manual/common" | ||
| + "github.com/juju/juju/environs/manual/linux" | ||
| + | ||
| + "github.com/juju/errors" |
| + | ||
| +// NewScriptProvisioner returns a valid ScriptProvisioner that will be used to produce the script. | ||
| +func NewScriptProvisioner(icfg *instancecfg.InstanceConfig) (common.ScriptProvisioner, error) { | ||
| + seriesos, err := series.GetOSFromSeries(icfg.Series) |
kat-co
Oct 19, 2016
Contributor
The OS is a good candidate for being part of an InstanceConfig, please add it there.
As an aside, I am performing some other work which will also use an OS stored in an InstanceConfig.
| + if err != nil { | ||
| + return nil, err | ||
| + } | ||
| + switch seriesos { |
| + return linux.NewScript(icfg), nil | ||
| + default: | ||
| + return nil, errors.NotFoundf( | ||
| + "Can't return a valid object for preparing the provisioning script based on this series %q", |
kat-co
Oct 19, 2016
Contributor
Please stick to standard error formats (e.g. "cannot blah blah"), also please try the errors you're creating out. Currently this will read as:
Can't return a valid object for preparing the provisioning script based on this series "xenial" not found
Also, the error message should reference the OS, not the series since it's the OS that drives the error, not the series. I would suggest: errors.NotSupportedf(seriesos) which will output:
Ubuntu not supported
Also, don't we need to support Windows here? This is in the top-level directory and not the linux package. Is it in the wrong place?
| + | ||
| +var _ = gc.Suite(&generateSuite{}) | ||
| + | ||
| +func (g *generateSuite) TestScript(c *gc.C) { |
kat-co
Oct 19, 2016
Contributor
The test name needs to relate the expected behavior and what is being tested. It looks like this test is attempting to check whether supported OSs do not error. So I would suggest TestNewScriptProvisioner_SupportedOSDontErr.
Also, this test is doing too much. Tests should document our expectations, and check one thing only. This should be broken into two tests:
TestNewScriptProvisioner_SupportsUbuntuTestNewScriptProvisioner_SupportsCentOS
| + c.Assert(ok, jc.IsTrue) | ||
| + c.Assert(val, gc.NotNil) | ||
| + } | ||
| + // this is supposed to fail |
kat-co
Oct 19, 2016
Contributor
Please separate this into a different test. Suggested name: TestNewScriptProvisioner_UnknownOSErrors.
| import ( | ||
| "fmt" | ||
| "io/ioutil" | ||
| "path/filepath" | ||
| "strings" | ||
| + "github.com/juju/juju/environs/manual/linux" |
| @@ -10,54 +11,90 @@ import ( | ||
| "strconv" | ||
| "strings" | ||
| + "github.com/juju/juju/apiserver/params" |
| +// | ||
| +// stdin and stdout will be used for remote sudo prompts, | ||
| +// if the ubuntu user must be created/updated. | ||
| +func InitUbuntuUser(host, login, authorizedKeys string, stdin io.Reader, stdout io.Writer) error { |
kat-co
Oct 19, 2016
Contributor
We don't need to know that it's stdin and stdout. We just need to know that we have something we can read/write from/to.
| +umask 0077 | ||
| +temp=$(mktemp) | ||
| +echo 'ubuntu ALL=(ALL) NOPASSWD:ALL' > $temp | ||
| +install -m 0440 $temp /etc/sudoers.d/90-juju-ubuntu |
kat-co
Oct 19, 2016
Contributor
Even though this is all Linux, this needs be configurable. Pass in a configPath variable that is derived from our github.com/juju/juju/juju/paths package up the stack.
hoenirvili
Oct 20, 2016
Contributor
Can you supply more information? The advice here is to replace /etc/sudoers.d/90-juju-ubuntu with %s and sprintf with the resulf of a func from the path pkg?
| + output := strings.TrimSpace(stdout.String()) | ||
| + provisioned := strings.Contains(output, "juju") | ||
| + if provisioned { | ||
| + logger.Infof("%s is already provisioned [%q]", host, output) |
| + | ||
| + provisioned, err := checkProvisioned(hostname) | ||
| + if err != nil { | ||
| + err = fmt.Errorf("error checking if provisioned: %v", err) |
| @@ -12,7 +12,7 @@ import ( | ||
| func Test(t *stdtesting.T) { | ||
| if runtime.GOOS == "windows" { | ||
| - t.Skip("Manual provider is not supported on windows") | ||
| + t.Skip("Manual provider as client is not supported on windows") |
kat-co
Oct 19, 2016
Contributor
Change this file to be guarded by a build flag instead: // +build linux
| +package linux | ||
| + | ||
| +import ( | ||
| + "github.com/juju/juju/apiserver/params" |
| + | ||
| +const Scope = "ssh" | ||
| + | ||
| +type provision struct { |
kat-co
Oct 19, 2016
Contributor
provisioner? We have NewProvisioner(), and generally structs are nouns, not verbs.
Why is this a struct? It has no member variables and only one method. Can we just have a function instead?
hoenirvili
Oct 20, 2016
Contributor
I'm using this struct because this patch was made from another big patch, that I intent to soon release this main PR.
hoenirvili#8
with this patch also in utils.
https://github.com/hoenirvili/utils/commit/0a87d413aff364e0d0cf96bfeb90f5d894acbead
The patch enables manual provisioning for windows machines. In this moment, we can manually provision just linux like machines. @kat-coif you look closely in the manual/provisioner.go we have this interface to let us more easily add more implementations for this process of provisioning.
// Provisioner is the process of provisioning a windows/linux machine
type Provisioner interface {
// ProvisionMachine is the main entrypoint of the Provisioner
ProvisionMachine(params common.ProvisionMachineArgs) (machineId string, err error)
} | + | ||
| +// Provision returns a new machineId and nil if the provision process is done successfully | ||
| +// The func will manual provision a linux machine using as it's default protocol SSH | ||
| +func (p provision) ProvisionMachine(args common.ProvisionMachineArgs) (machineId string, err error) { |
kat-co
Oct 19, 2016
Contributor
If we keep it a method, drop the p as it's not used. However, this is almost always an indication that it should not be a method.
| +// Provision returns a new machineId and nil if the provision process is done successfully | ||
| +// The func will manual provision a linux machine using as it's default protocol SSH | ||
| +func (p provision) ProvisionMachine(args common.ProvisionMachineArgs) (machineId string, err error) { | ||
| + defer func() { |
kat-co
Oct 19, 2016
Contributor
This isn't something that should go in a defer statement. This should be normal handled in the normal error-path of this method.
| + } | ||
| + | ||
| + // Finally, provision the machine agent. | ||
| + err = runProvisionScript(provisioningScript, args.Host, args.Stderr) |
kat-co
Oct 19, 2016
Contributor
Keep the construction of the script separate from the running of the script.
| envtesting "github.com/juju/juju/environs/testing" | ||
| envtools "github.com/juju/juju/environs/tools" | ||
| "github.com/juju/juju/instance" | ||
| "github.com/juju/juju/juju/testing" | ||
| + |
| "github.com/juju/juju/instance" | ||
| - "github.com/juju/juju/state/multiwatcher" | ||
| + "github.com/juju/loggo" |
| Host string | ||
| + User string |
| @@ -42,7 +43,7 @@ const ( | ||
| var ( | ||
| logger = loggo.GetLogger("juju.provider.manual") | ||
| - manualCheckProvisioned = manual.CheckProvisioned | ||
| + manualCheckProvisioned = linux.CheckProvisioned |
kat-co
Oct 19, 2016
Contributor
I think something needs to be reworked here; the abstraction (poor as it was: global variable) doesn't seem to fit any longer.
|
@axw @kat-co |
axw
requested changes
Oct 25, 2016
Sorry, I'm -1 on the API. The OS and method of provisioning are being conflated, which muddies the concepts. This was the case before, but reversed (ssh implied linux; now linux implies ssh). The new way makes more sense, but the lack of separation means things are not separated by concern, and cannot be easily ported to additional operating systems.
I would like the API to look more like this:
- environs/manual: common bits for manual provisioners
- environs/manual/sshprovisioner: ssh-specific manual provisioning code
- environs/manual/winrmprovisioner: winrm-specific manual provisioning code
environs/manual/sshprovisioner and environs/manual/winrmprovisioner should depend on environs/manual, not the other way around. Either the CLI code should hold the logic for calling into the appropriate package based on the placement scope, or we introduce some sort of registry. Either way, there's no cycle of cmd -> environs/manual -> ssh/winrm -> environs/manual.
In theory, sshprovisioner could be made to work with Windows, but I don't think we need to support that. Not in the near term anyway. What's more likely is that we would add support for some other *nix, which is why the detection script ought to live inside sshprovisioner, and not a linux package.
| +) | ||
| + | ||
| +// NewScriptProvisioner returns a valid ScriptProvisioner that will be used to produce the script. | ||
| +func NewScriptProvisioner(icfg *instancecfg.InstanceConfig) (common.ScriptProvisioner, error) { |
axw
Oct 25, 2016
Member
I don't see the point of the ScriptProvisioner interface. Why not just have:
func ProvisioningScript(icfg *instancecfg.InstanceConfig) (string, error) {
...
switch seriesOS {
case os.Ubuntu, os.CentOS, os.GenericLinux:
return linux.ProvisioningScript(icfg), nil
...
}
}
?
axw
requested changes
Oct 27, 2016
Looking better, thanks. Apart from these things, please make sure the imports are grouped as:
import (
stdlib
thirdparty
juju/juju/...
)
Within the juju/juju repository, we consider even juju/ packages (such as juju/utils) to be "third party".
| @@ -393,12 +392,13 @@ func (c *Client) ProvisioningScript(args params.ProvisioningScriptParams) (param | ||
| icfg.EnableOSRefreshUpdate = cfg.EnableOSRefreshUpdate() | ||
| } | ||
| - result.Script, err = manual.ProvisioningScript(icfg) | ||
| + result.Script, err = sshprovisioner.ProvisioningScript(icfg) |
axw
Oct 27, 2016
Member
I think we might want the ProvisioningScript function inside a linux-specific package, because it's independent of the method of execution. Hypothetically, you should be able to execute that script via os/exec just as well as via ssh.
What I imagined was: CLI calls into sshprovisioner, which detects the target machine's series, arch, etc. It then calls environs/manual.ProvisioningScript with that detected information, which calls into an OS-specific function to generate the script. i.e. something like:
CLI -> sshprovisioner|winrmprovisioner -> environs/manual.ProvisioningScript -> environs/manual/{linux,windows,...}.ProvisioningScript
Leave it for now, but I think it may make sense to shuffle it around again later.
| +// | ||
| +// we use manualProvisioner as a lambda and not a function | ||
| +// because this variable will be overwritten in tests. | ||
| +var manualProvisioner = func(args manual.ProvisionMachineArgs, |
axw
Oct 27, 2016
Member
I think all of this should be in the "scopeRun" function. We should only be mocking at the boundaries, i.e. manual.ProvisionMachine/sshprovisioner.New.
| +var manualProvisioner = func(args manual.ProvisionMachineArgs, | ||
| + placement *instance.Placement) (machineId string, err error) { | ||
| + defer func() { | ||
| + if machineId != "" && err != nil { |
axw
Oct 27, 2016
Member
If the provisioning function returns an error, we should disregard any other results. The provisioning function needs to be responsible for cleaning up after itself.
| + | ||
| + var p manual.Provisioner | ||
| + switch placement.Scope { | ||
| + case sshprovisioner.Scope: |
axw
Oct 27, 2016
Member
If nothing else is using sshprovisioner.Scope/manual.ErrNoProtoScope, I'd prefer if we kept them in this package. If/when we need to use them elsewhere, we can consider where they'd be best placed.
| @@ -341,3 +364,32 @@ func (c *addCommand) Run(ctx *cmd.Context) error { | ||
| } | ||
| return nil | ||
| } | ||
| + | ||
| +func (c *addCommand) scopeRun(client AddMachineAPI, config *config.Config, ctx *cmd.Context) error { |
axw
Oct 27, 2016
Member
"scopeRun" doesn't really explain what it's doing. How about "maybeManuallyProvisionMachine"?
axw
Oct 27, 2016
Member
I think you should move all of the code from "manualProvisioner" into here, so it looks like:
var sshprovisionerProvisionMachine = sshprovisioner.ProvisionMachine
var errNonManualScope = errors.New("non-manual scope")
func (c *addCommand) maybeManuallyProvisionMachine(client AddMachineAPI, config *config.Config, ctx *cmd.Context) error {
var provisionMachine func(manual.ProvisionMachineArgs) (string, error)
switch c.Placement.Scope {
case "ssh":
manualProvisioner = sshprovisionerProvisionMachine
default:
return errNonManualScope
}
logger.Infof("manual provisioning (%q)", c.Placement.Scope)
authKeys, err ...
user, host ...
args := ...
machineId, err := provisionMachine(args)
...
}| - } | ||
| - return sshinit.RunConfigureScript(script, params) | ||
| +// Provisioner is the process of provisioning a windows/linux machine | ||
| +type Provisioner interface { |
axw
Oct 27, 2016
Member
I don't think this interface is necessary for now. Please just have top-level functions in the sshprovisioner/winrmprovisioner packages. We may later want to have something stateful, but for now it's just additional boilerplate for no benefit.
It would be useful to have a type for the function though:
type ProvisionMachineFunc func(ProvisionMachineArgs) (machineId string, err error)
axw
Oct 27, 2016
Member
Also, please put the definition of ProvisionMachineArgs just below, and ProvisioningClientAPI below that. They're part of the interface.
| +// Copyright 2013 Canonical Ltd. | ||
| +// Licensed under the AGPLv3, see LICENCE file for details. | ||
| + | ||
| +// +build linux |
axw
Oct 27, 2016
Member
Can you please have it build on all, but just t.Skip in the Test function? That way it's more obvious why the tests aren't running.
kat-co
Oct 28, 2016
Contributor
Glancing through this, but I think I asked him to do this. No need to build if it's never intended to run. Our tests are long enough ;)
I don't feel too strongly about this.
axw
Oct 30, 2016
Member
@kat-co @hoenirvili I don't think we should be skipping everything (anything?) in that package on Windows. This code is run from the client, so it shouldn't matter what OS it is. We should be fixing the tests to be cross-platform, rather than not building them. So please use skip so it's in our faces.
| + | ||
| +import stdtesting "testing" | ||
| + | ||
| +func Test(t *stdtesting.T) { |
axw
Oct 27, 2016
Member
This needs to run the gocheck tests. Why don't you just copy suite_test.go over as it was, and rename it to package_test.go?
| + | ||
| +// Provision returns a new machineId and nil if the provision process is done successfully | ||
| +// The func will manual provision a linux machine using as it's default protocol SSH | ||
| +func (provisioner) ProvisionMachine(args manual.ProvisionMachineArgs) (machineId string, err error) { |
axw
Oct 27, 2016
Member
As mentioned elsewhere, please make this a top-level function func ProvisionMachine.
|
!!build!! |
|
!!build!! |
|
!!build!! |
| +var ( | ||
| + // ErrProvisioned is returned by ProvisionMachine if the target | ||
| + // machine has an existing machine agent. | ||
| + ErrProvisioned = errors.New("machine is already provisioned") |
axw
Oct 30, 2016
Member
Sorry I missed this before, this should also go in the other file, as it is also part of the API.
|
I'll leave this for a couple of days to make sure @kat-co is satisfied. @hoenirvili please let us know when this is ready to merge. |
|
Thanks for sticking with it, @hoenirvili! LGTM |
|
$$merge$$ |
|
$$merge$$ |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
hoenirvili commentedOct 10, 2016
This patch contains modifications in the linux manual provision code path.
It makes the code much more robust, makes it more flexible through changes adding more interfaces.
After this patch, we can easily add multiple transparent ways of manual provisioning a
machine therefore, making it easier to enable manual provisioning for windows
machines.(this will be followed by the patch for manual windows provisioning).