Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(468)

Issue 28270043: cmd/jujud: remove mongo and data-dir on teardown

Can't Edit
Can't Publish+Mail
Start Review
Created:
5 years, 12 months ago by axw
Modified:
5 years, 12 months ago
Reviewers:
jameinel, mp+195565, rog
Visibility:
Public.

Description

cmd/jujud: remove mongo and data-dir on teardown Two new agent.conf keys are introduced: - AGENT_SERVICE_NAME, and - MONGO_SERVICE_NAME. The former is always added, the latter only for state servers. For backwards-compatibility we fall back to os.Getenv("UPSTART_JOB") for AGENT_SERVICE_NAME. https://code.launchpad.net/~axwalk/juju-core/jujud-uninstall-mongo/+merge/195565 (do not edit description out of merge proposal)

Patch Set 1 #

Total comments: 6

Patch Set 2 : cmd/jujud: remove mongo and data-dir on teardown #

Total comments: 6

Patch Set 3 : cmd/jujud: remove mongo and data-dir on teardown #

Unified diffs Side-by-side diffs Delta from patch set Stats (+77 lines, -11 lines) Patch
A [revision details] View 1 2 1 chunk +2 lines, -0 lines 0 comments Download
M agent/agent.go View 1 1 chunk +2 lines, -0 lines 0 comments Download
M cmd/jujud/machine.go View 1 2 1 chunk +29 lines, -7 lines 0 comments Download
M cmd/jujud/machine_test.go View 1 chunk +6 lines, -2 lines 0 comments Download
M environs/cloudinit/cloudinit.go View 1 4 chunks +13 lines, -2 lines 0 comments Download
M environs/cloudinit/cloudinit_test.go View 1 2 3 chunks +23 lines, -0 lines 0 comments Download
M provider/local/environ.go View 1 1 chunk +2 lines, -0 lines 0 comments Download

Messages

Total messages: 6
axw
Please take a look.
5 years, 12 months ago (2013-11-18 09:48:50 UTC) #1
jameinel
Did you test this works live? It looks reasonable to me, and seems a lot ...
5 years, 12 months ago (2013-11-18 09:56:46 UTC) #2
axw
> Did you test this works live? Yes indeed. Works with the null provider. Doesn't ...
5 years, 12 months ago (2013-11-19 08:22:54 UTC) #3
axw
Please take a look.
5 years, 12 months ago (2013-11-19 09:13:38 UTC) #4
rog
LGTM with some minor thoughts below. https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go File cmd/jujud/machine.go (right): https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newcode316 cmd/jujud/machine.go:316: errors = append(errors, ...
5 years, 12 months ago (2013-11-19 09:50:39 UTC) #5
axw
5 years, 12 months ago (2013-11-19 10:23:10 UTC) #6
Please take a look.

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go
File cmd/jujud/machine.go (right):

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:316: errors = append(errors, err)
On 2013/11/19 09:50:39, rog wrote:
> I think that having a bunch of errors without context might prove very hard
for
> a user to interpret if something goes wrong.
> 
> I think things might be more understandable if we added some context to each
> one;
> e.g.
> 
> errors = append(errors, fmt.Errorf("cannot remove service %q: %v",
> agentServiceName, err))

Good call. Done.

https://codereview.appspot.com/28270043/diff/20001/cmd/jujud/machine.go#newco...
cmd/jujud/machine.go:334: }
On 2013/11/19 09:50:39, rog wrote:
> if len(errors) == 1 {
>     return fmt.Errorf("uninstall failed: %v", errors[0])
> }
> 
> to save the square brackets when only one thing fails?
> probably not worth it, just a thought.

I concluded it wasn't worthwhile, as it made the code pretty ugly. The error
message will only ever go to the machine log.

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudin...
File environs/cloudinit/cloudinit.go (right):

https://codereview.appspot.com/28270043/diff/20001/environs/cloudinit/cloudin...
environs/cloudinit/cloudinit.go:313: acfg.SetValue(agent.MongoServiceName,
mongoServiceName)
On 2013/11/19 09:50:39, rog wrote:
> Is there nowhere to test this functionality?

Gah, I intended to do that, it just slipped my mind. Added a test.
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b