Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow add-machine to take a -n param #198

Merged
merged 4 commits into from Jul 4, 2014
Merged

Conversation

mattyw
Copy link
Contributor

@mattyw mattyw commented Jun 30, 2014

Fix for https://bugs.launchpad.net/juju-core/+bug/1214209

Allow add-machine to take a -n param to add multiple machines

@axw
Copy link
Contributor

axw commented Jul 1, 2014

I think you meant https://bugs.launchpad.net/juju-core/+bug/1214209

This needs some tweaking to take into account placement. I think if if the user specifies -n, then they probably shouldn't be allowed to specify a placement directive (just as in juju deploy).

@mattyw
Copy link
Contributor Author

mattyw commented Jul 2, 2014

@axw ptal

juju add-machine lxc (starts a new machine with an lxc container)
juju add-machine lxc -n 2 (starts 2 new machines with an lxc container)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This won't work. "lxc" is a placement directive.

EDIT: ah, I see what you did now. Never mind.

@axw
Copy link
Contributor

axw commented Jul 2, 2014

I think we could do with better error reporting, but otherwise I'm happy with this.

@mattyw
Copy link
Contributor Author

mattyw commented Jul 2, 2014

@axw thanks for the feedback, the output is much better now, ptal

}
}
if len(errs) == 1 {
return errs[0]
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm +1 on returning the original error if there's only one failure, but I think we need to be consistent with the error output.

I think it might be better just to print out the "failed to create N machine(s)" regardless of how many there are, and then return cmd.ErrSilent. Also, I think you want to be printing to ctx.Stderr() for errors -- they shouldn't be hidden by -q IMO.

@axw
Copy link
Contributor

axw commented Jul 3, 2014

Thanks. Nearly there, just would like the error output to be consistent between the single/multi error case.

@mattyw
Copy link
Contributor Author

mattyw commented Jul 3, 2014

@axw thanks for the feedback, ptal
Changes:

  • Single and multiple modes now print "failed to create 1/n machine/machines
  • The failed to create message is sent to stderr
  • In multiple failures the error message is joined with "," and printed in a single line. I experimented with other ways of displaying it but none of them appeared great in the log. The current version has the potential for long error lines but looks better than \n in my opinion

@axw
Copy link
Contributor

axw commented Jul 4, 2014

Thanks! LGTM

@mattyw
Copy link
Contributor Author

mattyw commented Jul 4, 2014

$$merge$$

@jujubot
Copy link
Collaborator

jujubot commented Jul 4, 2014

Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju

jujubot added a commit that referenced this pull request Jul 4, 2014
Allow add-machine to take a -n param

Fix for https://bugs.launchpad.net/juju-core/+bug/1214209

Allow add-machine to take a -n param to add multiple machines
@jujubot jujubot merged commit 74d46c5 into juju:master Jul 4, 2014
@mattyw mattyw deleted the add-n-machines branch July 24, 2014 05:35
laszlokajtar pushed a commit to laszlokajtar/juju that referenced this pull request Oct 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants