cmds/update: use new cmds lib #5730
Conversation
|
|
||
| // Get the node iff already defined. | ||
| if req.InvocContext().Online { |
There was a problem hiding this comment.
This check is important. We should onlu get the node if-and-only-if (iff) it is already defined.
There was a problem hiding this comment.
@kevina Should we add a helper function cmdenv.IsOnline()?
There was a problem hiding this comment.
Is it used in more than one or two places? Is do I would define one. Otherwise I wouldn't bother.
There was a problem hiding this comment.
@kevina Could u help me review it again?
overbool
force-pushed
the
refactor/cmds/update
branch
4 times, most recently
from
1c4844e
to
db08d26
Compare
There was a problem hiding this comment.
This LGTM but I would like @Stebalien to have a closer look before merging.
overbool
force-pushed
the
refactor/cmds/update
branch
from
db08d26
to
90aae6e
Compare
Stebalien
force-pushed
the
refactor/cmds/update
branch
from
90aae6e
to
1d65315
Compare
ghost
assigned
Stebalien
force-pushed
the
refactor/cmds/update
branch
2 times, most recently
from
d2b83ed
to
d51bcc6
Compare
License: MIT Signed-off-by: Overbool <overbool.xu@gmail.com>
Stebalien
force-pushed
the
refactor/cmds/update
branch
from
d51bcc6
to
5002d59
Compare
|
So, I went ahead and managed to blow away that last change (review change) when rebasing (fetched the wrong remote and saw that everything was up-to-date). However, I believe I've restored the change. |
Ref: #5664
License: MIT
Signed-off-by: Overbool overbool.xu@gmail.com