Join GitHub today
GitHub is home to over 20 million developers working together to host and review code, manage projects, and build software together.
Inform user of storage destruction/detachment in remove-{application,unit} #7100
Conversation
wallyworld
approved these changes
Mar 16, 2017
Awesome to remove JujuConnSuite. I think we need a couple of feature tests though to replace the now deleted full stack tests.
| -// Destroy destroys a given application. | ||
| -func (c *Client) Destroy(application string) error { | ||
| +// DestroyUnit decreases the number of units dedicated to an application. | ||
| +func (c *Client) DestroyUnit(unitNames ...string) ([]params.DestroyUnitResult, error) { |
axw
Mar 16, 2017
•
Edited 1 time
-
axw
Mar 16, 2017
Member
-
Mar 16, 2017
Yeah, misremembered the branch. After adding it I don't like it there either, but I'll make it plural for now.
| + } | ||
| + for i, name := range unitNames { | ||
| + if !names.IsValidUnit(name) { | ||
| + return nil, errors.NotValidf("unit name %q", name) |
wallyworld
Mar 16, 2017
Owner
This should set the error in that unit's Result struct.
One bad unit name shouldn't prevent the rest from being destroyed?
axw
Mar 16, 2017
Member
IMO it's OK here because the user has specified something invalid, rather than there being a server-side error for one of the parameters. I'll do it anyway, but question the value.
| params := params.ApplicationDestroy{ | ||
| ApplicationName: application, | ||
| } | ||
| return c.facade.FacadeCall("Destroy", params, nil) | ||
| } | ||
| +// DestroyApplication destroys the given applications. | ||
| +func (c *Client) DestroyApplication(appNames ...string) ([]params.DestroyApplicationResult, error) { |
| + } | ||
| + for i, name := range appNames { | ||
| + if !names.IsValidApplication(name) { | ||
| + return nil, errors.NotValidf("application name %q", name) |
| - jujutesting.JujuConnSuite | ||
| - | ||
| - client *application.Client | ||
| + testing.IsolationSuite |
| @@ -36,3 +37,34 @@ func (client *Client) AddMachines(machineParams []params.AddMachineParams) ([]pa | ||
| } | ||
| return results.Machines, err | ||
| } | ||
| + | ||
| +// DestroyMachines removes a given set of machines. | ||
| +func (client *Client) DestroyMachines(machines ...string) ([]params.DestroyMachineResult, error) { |
wallyworld
Mar 16, 2017
Owner
Can you add a todo to the CLI to use this new api instead of the one on the client facade?
And a deprecation todo on the client facade.
| + | ||
| +// DeployApplication is a wrapper around juju.DeployApplication, to | ||
| +// match the function signature expected by NewAPI. | ||
| +func DeployApplication(backend Backend, args jjj.DeployApplicationParams) error { |
wallyworld
Mar 16, 2017
Owner
Personally, I'd rather this not be exported.
This is a case where adding an alias to export_test makes sense IMO.
axw
Mar 16, 2017
Member
And I'd rather we didn't have hard-coded dependencies. Exporting this is a step towards being able to pass the dependency in, but providing an easy to get to default.
| + if err != nil { | ||
| + return nil, err | ||
| + } | ||
| + name := unitTag.Id() | ||
| unit, err := api.backend.Unit(name) |
| @@ -342,15 +342,15 @@ func opClientAddServiceUnits(c *gc.C, st api.Connection, mst *state.State) (func | ||
| } | ||
| func opClientDestroyServiceUnits(c *gc.C, st api.Connection, mst *state.State) (func(), error) { | ||
| - err := application.NewClient(st).DestroyUnits("wordpress/99") | ||
| + err := application.NewClient(st).DestroyUnitsDeprecated("wordpress/99") |
| - client, err := c.getAPI() | ||
| - if err != nil { | ||
| - return err | ||
| +func (c *removeApplicationCommand) removeApplicationsDeprecated( |
| + logger.Warningf("%s", err) | ||
| + continue | ||
| + } | ||
| + ctx.Infof("- removing %s", names.ReadableString(storageTag)) |
wallyworld
Mar 16, 2017
Owner
Having it worded as "removing" will be confusing, because for destroy-controller, we say "destroying machine" or whatever and then when that all is done, the job is finished. But here that same expectation doesn't hold true.
I think "will remove blah" is better as it better communicates the async semantics of the operation.
axw
Mar 16, 2017
Member
are you suggesting this?
removing application foo
- will remove storage bar/0
- will detach storage baz/1
?
wallyworld
Mar 16, 2017
Owner
yeah, something to indicate that when the commend ends, it may not have happened yet
I believe we have enough full-stack tests in cmd/juju/application already. |
|
Full stack tests in cmd/juju/application good - so long as we remember to add feature tests when those cmd tests are made into proper unit tests |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
|
Build failed: Tests failed |
|
$$merge$$ |
|
Status: merge request accepted. Url: http://juju-ci.vapour.ws:8080/job/github-merge-juju |
axw commentedMar 15, 2017
Description of change
We introduce new DestroyApplication and DestroyUnit
methods to the Application facade. These supersede
the existing Destroy and DestroyUnits methods,
respectively. The new methods are bulk, and return
information relating to the entities being destroyed.
This enables us to inform the user of storage that is
destroyed, or detached and left in the model, when
removing an application or unit with the remove-application
or remove-unit commands.
We will do the same for remove-machine in a follow-up.
Also, move a few tests over to the unit test suite in
apiserver/application.
QA steps
should see:
should see:
Also tested backwards compatibility (new server & old client, and vice versa).
Documentation changes
Bug reference
None.