Issue 14486043: state: Add SetEnvironAgentVersion (Closed)
Descriptionstate: Add SetEnvironAgentVersion
Implemented a SetEnvironAgentVersion state call, which:
1. Ensures the current environment has a valid agent-version;
2. Finds all machines and units in state that have an empty
or different tools version;
3. Changes the agent-version in the environment, asserting it
haven't changed in the mean time.
This is a prerequisite step to changing upgrage-juju command
to default to major.minor+2 steps, unless forced with --version.
https://code.launchpad.net/~dimitern/juju-core/160-setenvironagentversion/+merge/189570
(do not edit description out of merge proposal)
Patch Set 1 #Patch Set 2 : state: Add SetEnvironAgentVersion #
Total comments: 38
Patch Set 3 : state: Add SetEnvironAgentVersion #
Total comments: 10
Patch Set 4 : state: Add SetEnvironAgentVersion #
Total comments: 10
Patch Set 5 : state: Add SetEnvironAgentVersion #
Total comments: 6
Patch Set 6 : state: Add SetEnvironAgentVersion #
MessagesTotal messages: 16
Please take a look.
Sign in to reply to this message.
Please take a look.
Sign in to reply to this message.
Looking good; some thoughts and suggestions below. https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode147 state/state.go:147: machines map[string]version.Number Why not just have a single map that uses tags? Then the logic in Error can be quite a bit simpler, I think. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode153 state/state.go:153: items := make([]string, len(m)) items := make([]string, 0, len(m)) then you can just append to items and lose the i index. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode158 state/state.go:158: vers = "N/A" s/N\/A/unset/ ? https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode166 state/state.go:166: message := fmt.Sprintf("current environment version %s is inconsistent for", e.currentVersion) Perhaps "some agents have not updated to the current version" might be a more user-understandable way of phrasing this error? https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode172 state/state.go:172: return fmt.Sprintf("%s units %v", message, str(e.units)) To be honest, I'm not sure that all the logic here is worth it. The versions are likely to be changing soon anyway. Just a list of the offending agent tags might be sufficient. e.g. cannot upgrade juju: some agents have not updated to the current version: machine-0 machine-1 unit-wordpress-0 The user can always use juju status to find the actual current versions. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode177 state/state.go:177: func NewVersionInconsistentError( I don't think this needs to be exported. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode258 state/state.go:258: Assert: D{{"agent-version", currentVersion}}, Is this a strong enough assertion? I know it's pretty unlikely but in theory couldn't something set the version to something different, start a new machine which starts to download that version, then change the version back to what it was before? Perhaps it's not worth worrying about, but I wonder if a revid assertion might give more peace of mind. https://codereview.appspot.com/14486043/diff/6001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state_test.go#newcode... state/state_test.go:1893: expectErr = fmt.Sprintf("current environment version %s is inconsistent for machines 0: 9.9.9, 1: N/A and units wordpress/0: 6.6.6, wordpress/1: N/A", stringVersion) This looks flaky to me. Isn't the order of the units in the error message arbitrary, because it creates them by iterating over the map? I think you probably want to sort the keys in the Error method. https://codereview.appspot.com/14486043/diff/6001/state/state_test.go#newcode... state/state_test.go:1894: c.Assert(err, gc.ErrorMatches, expectErr) c.Assert(err, jc.Satisfies, state.IsVersionInconsistentError) ?
Sign in to reply to this message.
Few thoughts, general agreement with rog https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode172 state/state.go:172: return fmt.Sprintf("%s units %v", message, str(e.units)) On 2013/10/07 12:28:55, rog wrote: > To be honest, I'm not sure that all the logic here is worth it. The versions are > likely to be changing soon anyway. Just a list of the offending agent tags might > be sufficient. > e.g. > cannot upgrade juju: some agents have not updated to the current version: > machine-0 machine-1 unit-wordpress-0 > > The user can always use juju status to find the actual current versions. List of tags sounds good. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode208 state/state.go:208: } if currentVersion == newVersion { return nil } https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode210 state/state.go:210: matchVersion := "^" + regexp.QuoteMeta(currentVersion) + "-.*" I think that .* there kills performance, doesn't it? Check the docs for the restrictions on fast regex queries. And probably figure out how to index them, I think :). https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode214 state/state.go:214: {{"tools.version", D{{"$not", bson.RegEx{matchVersion, "i"}}}}}, Heh. It crosses my mind that we should probably also accept agents that are already running the requested version. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode218 state/state.go:218: unitDocs []unitDoc how about just a toolsDoc? not sure we care about the rest (apart from _id...) https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode254 state/state.go:254: settings.Set("agent-version", newVersion.String()) What's this line for? https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode258 state/state.go:258: Assert: D{{"agent-version", currentVersion}}, On 2013/10/07 12:28:55, rog wrote: > Is this a strong enough assertion? > I know it's pretty unlikely but in theory couldn't > something set the version to something different, > start a new machine which starts to download that version, then > change the version back to what it was before? > > Perhaps it's not worth worrying about, but I wonder > if a revid assertion might give more peace of mind. +1 on txn-revno check
Sign in to reply to this message.
https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode214 state/state.go:214: {{"tools.version", D{{"$not", bson.RegEx{matchVersion, "i"}}}}}, On 2013/10/07 13:03:58, fwereade wrote: > Heh. It crosses my mind that we should probably also accept agents that are > already running the requested version. I'm not sure that's worth doing. Just noticed: why a case-insensitive match?
Sign in to reply to this message.
On 2013/10/07 13:15:02, rog wrote: > On 2013/10/07 13:03:58, fwereade wrote: > > Heh. It crosses my mind that we should probably also accept agents that are > > already running the requested version. > > I'm not sure that's worth doing. It seems like it's the only way to unbreak a half-upgraded environment (in which tools are not available for all series/arches you've got deployed). It may be that the only-forwards assumption is enough to scotch us there in some cases regardless, but I'm not sure it's always the case. > Just noticed: why a case-insensitive match? No idea. Don't see much benefit there, indeed.
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode147 state/state.go:147: machines map[string]version.Number On 2013/10/07 12:28:55, rog wrote: > Why not just have a single map that uses tags? > Then the logic in Error can be quite a bit simpler, > I think. Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode153 state/state.go:153: items := make([]string, len(m)) On 2013/10/07 12:28:55, rog wrote: > items := make([]string, 0, len(m)) > > then you can just append to items and lose the i index. Why? I don't see a benefit in this - by reserving all the elements in advance and just iterating seems more efficient than doing appends, which allocate a new slice each time. Anyway, this is changed now to use a list of tags. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode158 state/state.go:158: vers = "N/A" On 2013/10/07 12:28:55, rog wrote: > s/N\/A/unset/ ? Removed with the introduction of the list of agent tags. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode166 state/state.go:166: message := fmt.Sprintf("current environment version %s is inconsistent for", e.currentVersion) On 2013/10/07 12:28:55, rog wrote: > Perhaps "some agents have not updated to the current version" might be a more > user-understandable way of phrasing this error? Rephrased. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode172 state/state.go:172: return fmt.Sprintf("%s units %v", message, str(e.units)) On 2013/10/07 12:28:55, rog wrote: > To be honest, I'm not sure that all the logic here is worth it. The versions are > likely to be changing soon anyway. Just a list of the offending agent tags might > be sufficient. > e.g. > cannot upgrade juju: some agents have not updated to the current version: > machine-0 machine-1 unit-wordpress-0 > > The user can always use juju status to find the actual current versions. Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode172 state/state.go:172: return fmt.Sprintf("%s units %v", message, str(e.units)) On 2013/10/07 13:03:58, fwereade wrote: > On 2013/10/07 12:28:55, rog wrote: > > To be honest, I'm not sure that all the logic here is worth it. The versions > are > > likely to be changing soon anyway. Just a list of the offending agent tags > might > > be sufficient. > > e.g. > > cannot upgrade juju: some agents have not updated to the current version: > > machine-0 machine-1 unit-wordpress-0 > > > > The user can always use juju status to find the actual current versions. > > List of tags sounds good. Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode177 state/state.go:177: func NewVersionInconsistentError( On 2013/10/07 12:28:55, rog wrote: > I don't think this needs to be exported. Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode208 state/state.go:208: } On 2013/10/07 13:03:58, fwereade wrote: > if currentVersion == newVersion { > return nil > } Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode210 state/state.go:210: matchVersion := "^" + regexp.QuoteMeta(currentVersion) + "-.*" On 2013/10/07 13:03:58, fwereade wrote: > I think that .* there kills performance, doesn't it? Check the docs for the > restrictions on fast regex queries. And probably figure out how to index them, I > think :). According to mongo docs, we need anchoring (^), case-sensitive match and no .* for best performance. Changed accordingly. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode214 state/state.go:214: {{"tools.version", D{{"$not", bson.RegEx{matchVersion, "i"}}}}}, On 2013/10/07 13:03:58, fwereade wrote: > Heh. It crosses my mind that we should probably also accept agents that are > already running the requested version. Good point! Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode214 state/state.go:214: {{"tools.version", D{{"$not", bson.RegEx{matchVersion, "i"}}}}}, On 2013/10/07 13:15:02, rog wrote: > On 2013/10/07 13:03:58, fwereade wrote: > > Heh. It crosses my mind that we should probably also accept agents that are > > already running the requested version. > > I'm not sure that's worth doing. > > Just noticed: why a case-insensitive match? Wasn't sure I can just pass "" for flags, but apparently I can. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode218 state/state.go:218: unitDocs []unitDoc On 2013/10/07 13:03:58, fwereade wrote: > how about just a toolsDoc? not sure we care about the rest (apart from _id...) There's no toolsDoc really - it's just a field of machine/unitDoc, and I'm not sure how can I get a subdocument like that from a set. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode254 state/state.go:254: settings.Set("agent-version", newVersion.String()) On 2013/10/07 13:03:58, fwereade wrote: > What's this line for? Oh, left over. Removed. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode258 state/state.go:258: Assert: D{{"agent-version", currentVersion}}, On 2013/10/07 12:28:55, rog wrote: > Is this a strong enough assertion? > I know it's pretty unlikely but in theory couldn't > something set the version to something different, > start a new machine which starts to download that version, then > change the version back to what it was before? > > Perhaps it's not worth worrying about, but I wonder > if a revid assertion might give more peace of mind. Done. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode258 state/state.go:258: Assert: D{{"agent-version", currentVersion}}, On 2013/10/07 13:03:58, fwereade wrote: > On 2013/10/07 12:28:55, rog wrote: > > Is this a strong enough assertion? > > I know it's pretty unlikely but in theory couldn't > > something set the version to something different, > > start a new machine which starts to download that version, then > > change the version back to what it was before? > > > > Perhaps it's not worth worrying about, but I wonder > > if a revid assertion might give more peace of mind. > > +1 on txn-revno check Done. https://codereview.appspot.com/14486043/diff/6001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state_test.go#newcode... state/state_test.go:1893: expectErr = fmt.Sprintf("current environment version %s is inconsistent for machines 0: 9.9.9, 1: N/A and units wordpress/0: 6.6.6, wordpress/1: N/A", stringVersion) On 2013/10/07 12:28:55, rog wrote: > This looks flaky to me. > Isn't the order of the units in the error message arbitrary, because it creates > them by iterating over the map? I think you probably want to sort the keys in > the Error method. Done. https://codereview.appspot.com/14486043/diff/6001/state/state_test.go#newcode... state/state_test.go:1894: c.Assert(err, gc.ErrorMatches, expectErr) On 2013/10/07 12:28:55, rog wrote: > c.Assert(err, jc.Satisfies, state.IsVersionInconsistentError) ? Done. Although, I still want to check the exact message.
Sign in to reply to this message.
LGTM module the below minor points. Is there some way of checking transaction behaviour now in the presence of violated assertions, BTW? https://codereview.appspot.com/14486043/diff/18001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode224 state/state.go:224: // hasn't changed in the mean time. s/mean time/meantime/ https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode232 state/state.go:232: return fmt.Errorf("cannot set agent-version: %v", err) I think if err is txn.ErrAborted we could give a better error message. eg. "cannot set agent version: environment settings have changed"
Sign in to reply to this message.
On 2013/10/08 12:42:26, rog wrote: > LGTM module the below minor points. s/module/modulo/
Sign in to reply to this message.
Couple more thoughts https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode153 state/state.go:153: items := make([]string, len(m)) On 2013/10/08 10:40:13, dimitern wrote: > On 2013/10/07 12:28:55, rog wrote: > > items := make([]string, 0, len(m)) > > > > then you can just append to items and lose the i index. > > Why? I don't see a benefit in this - by reserving all the elements in advance > and just iterating seems more efficient than doing appends, which allocate a new > slice each time. If you create it with the max expected capacity, as above, you won't reallocate. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode218 state/state.go:218: unitDocs []unitDoc On 2013/10/08 10:40:13, dimitern wrote: > On 2013/10/07 13:03:58, fwereade wrote: > > how about just a toolsDoc? not sure we care about the rest (apart from _id...) > > There's no toolsDoc really - it's just a field of machine/unitDoc, and I'm not > sure how can I get a subdocument like that from a set. I was suggesting you create one -- search for lifeDoc in watcher.go -- but I'll take a look at the new code and see if it's still relevant. https://codereview.appspot.com/14486043/diff/18001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode194 state/state.go:194: {{"tools", D{{"$exists", false}}}}, Do we definitely need this line? https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode212 state/state.go:212: } So, in fact, I think this just needs to be a one-field struct (with _id string) -- something like: type idDoc struct { Id string `bson:"_id"` } var machineDocs []idDoc st.machines.Find(sel).Select(D{{"life", 1}}).All(&machineDocs) ...which then lets you pull down just the ids. (although maybe an Iter() would be even nicer? up to you) https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode233 state/state.go:233: } I think this probably wants the usual retrying -- it can now fail for irrelevant reasons (eg someone set default-series).
Sign in to reply to this message.
On 2013/10/08 12:42:26, rog wrote: > LGTM module the below minor points. > > Is there some way of checking transaction > behaviour now in the presence of violated > assertions, BTW? There is -- SetTransactionHooks et al. We should use them :).
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/14486043/diff/6001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode153 state/state.go:153: items := make([]string, len(m)) On 2013/10/08 12:44:58, fwereade wrote: > On 2013/10/08 10:40:13, dimitern wrote: > > On 2013/10/07 12:28:55, rog wrote: > > > items := make([]string, 0, len(m)) > > > > > > then you can just append to items and lose the i index. > > > > Why? I don't see a benefit in this - by reserving all the elements in advance > > and just iterating seems more efficient than doing appends, which allocate a > new > > slice each time. > > If you create it with the max expected capacity, as above, you won't reallocate. Good to know, but after the changes, this is no longer relevant. https://codereview.appspot.com/14486043/diff/6001/state/state.go#newcode218 state/state.go:218: unitDocs []unitDoc On 2013/10/08 12:44:58, fwereade wrote: > On 2013/10/08 10:40:13, dimitern wrote: > > On 2013/10/07 13:03:58, fwereade wrote: > > > how about just a toolsDoc? not sure we care about the rest (apart from > _id...) > > > > There's no toolsDoc really - it's just a field of machine/unitDoc, and I'm not > > sure how can I get a subdocument like that from a set. > > I was suggesting you create one -- search for lifeDoc in watcher.go -- but I'll > take a look at the new code and see if it's still relevant. Good idea, will try to simplify based on that. https://codereview.appspot.com/14486043/diff/18001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode194 state/state.go:194: {{"tools", D{{"$exists", false}}}}, On 2013/10/08 12:44:58, fwereade wrote: > Do we definitely need this line? I think we do - how can we know there are unset versions in the collection otherwise? https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode212 state/state.go:212: } On 2013/10/08 12:44:58, fwereade wrote: > So, in fact, I think this just needs to be a one-field struct (with _id string) > -- something like: > > type idDoc struct { > Id string `bson:"_id"` > } > var machineDocs []idDoc > st.machines.Find(sel).Select(D{{"life", 1}}).All(&machineDocs) > > ...which then lets you pull down just the ids. (although maybe an Iter() would > be even nicer? up to you) Will give it a try. https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode224 state/state.go:224: // hasn't changed in the mean time. On 2013/10/08 12:42:26, rog wrote: > s/mean time/meantime/ Done. https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode232 state/state.go:232: return fmt.Errorf("cannot set agent-version: %v", err) On 2013/10/08 12:42:26, rog wrote: > I think if err is txn.ErrAborted we could give a > better error message. > > eg. "cannot set agent version: environment settings have changed" Added retrying logic. https://codereview.appspot.com/14486043/diff/18001/state/state.go#newcode233 state/state.go:233: } On 2013/10/08 12:44:58, fwereade wrote: > I think this probably wants the usual retrying -- it can now fail for irrelevant > reasons (eg someone set default-series). Done.
Sign in to reply to this message.
Coming along nicely, but I think there are some subtleties... https://codereview.appspot.com/14486043/diff/25001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode163 state/state.go:163: func IsVersionInconsistentError(e interface{}) bool { Hmm. Requiring this precise error makes ErrorContextf inappropriate below... but what do we actually get out of this func? https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode192 state/state.go:192: currentVersion, revNo, err := st.getCurrentAgentVersion(newVersion) Looks like there's some missing logic here... and there's a bit of a smell that you call the same thing again later. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode232 state/state.go:232: Assert: D{{"txn-revno", revNo}, {"agent-version", currentVersion}}, agent-version assert is impliedby the txn-revno assert, don;t need to check both. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode238 state/state.go:238: return fmt.Errorf("cannot set agent-version: %v", err) if you used ErrorContextf, you could just do `if err != txn.ErrAborted { return err }` here. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode239 state/state.go:239: } I think you need to redo all the logic you currently have outside the loop at this point, don't you? agent-version *could* have changed after all...
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/14486043/diff/25001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode163 state/state.go:163: func IsVersionInconsistentError(e interface{}) bool { On 2013/10/08 14:50:40, fwereade wrote: > Hmm. Requiring this precise error makes ErrorContextf inappropriate below... but > what do we actually get out of this func? We have a concrete error, we can check for and issue better error messages for the user for upgrade-juju later. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode192 state/state.go:192: currentVersion, revNo, err := st.getCurrentAgentVersion(newVersion) On 2013/10/08 14:50:40, fwereade wrote: > Looks like there's some missing logic here... and there's a bit of a smell that > you call the same thing again later. oh, yeah - the error should be checked, like in the abort handling below. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode232 state/state.go:232: Assert: D{{"txn-revno", revNo}, {"agent-version", currentVersion}}, On 2013/10/08 14:50:40, fwereade wrote: > agent-version assert is impliedby the txn-revno assert, don;t need to check > both. Done. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode238 state/state.go:238: return fmt.Errorf("cannot set agent-version: %v", err) On 2013/10/08 14:50:40, fwereade wrote: > if you used ErrorContextf, you could just do `if err != txn.ErrAborted { return > err }` here. I don't really like using ErrorContextf. https://codereview.appspot.com/14486043/diff/25001/state/state.go#newcode239 state/state.go:239: } On 2013/10/08 14:50:40, fwereade wrote: > I think you need to redo all the logic you currently have outside the loop at > this point, don't you? agent-version *could* have changed after all... Done.
Sign in to reply to this message.
LGTM modulo details of exactly how the code's arranged. Thanks. https://codereview.appspot.com/14486043/diff/16001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/16001/state/state.go#newcode226 state/state.go:226: if err == nil && currentVersion == "" { This is a little bit opaque, you might be better served by just poking getCurrentAgentVersion into this func directly now that there's just one client (plus, I don't like that name (given what it does) and can't think of a good one ;)). https://codereview.appspot.com/14486043/diff/16001/state/state.go#newcode239 state/state.go:239: } This block might make sense rolled into something like if err := st.checkCanUpgrade(cur,new); err != nil { return err } ? seems like a bit more of a cohesive operation than getAgentTagsNotMatchingVersion... https://codereview.appspot.com/14486043/diff/16001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/14486043/diff/16001/state/state_test.go#newcod... state/state_test.go:2010: checker.Check() Would be great to split this func into several for the various cases you're testing -- they seem basically independent, right?
Sign in to reply to this message.
Please take a look. https://codereview.appspot.com/14486043/diff/16001/state/state.go File state/state.go (right): https://codereview.appspot.com/14486043/diff/16001/state/state.go#newcode226 state/state.go:226: if err == nil && currentVersion == "" { On 2013/10/08 15:50:08, fwereade wrote: > This is a little bit opaque, you might be better served by just poking > getCurrentAgentVersion into this func directly now that there's just one client > (plus, I don't like that name (given what it does) and can't think of a good one > ;)). Done. https://codereview.appspot.com/14486043/diff/16001/state/state.go#newcode239 state/state.go:239: } On 2013/10/08 15:50:08, fwereade wrote: > This block might make sense rolled into something like > > if err := st.checkCanUpgrade(cur,new); err != nil { > return err > } > > ? seems like a bit more of a cohesive operation than > getAgentTagsNotMatchingVersion... Done. https://codereview.appspot.com/14486043/diff/16001/state/state_test.go File state/state_test.go (right): https://codereview.appspot.com/14486043/diff/16001/state/state_test.go#newcod... state/state_test.go:2010: checker.Check() On 2013/10/08 15:50:08, fwereade wrote: > Would be great to split this func into several for the various cases you're > testing -- they seem basically independent, right? I wanted to reuse the setup code - that's what they share, but I guess with a few helpers it'll split nicely in smaller tests.
Sign in to reply to this message.
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
