commands: reimplements printing deployment statuses, closes #462 #486
Conversation
ilgooz
commented
Sep 20, 2018
ilgooz
commented
- utils/pretty: add UseSpinner() and DestroySpinner() funcs.
* utils/pretty: add UseSpinner() and DestroySpinner() funcs.
ilgooz
force-pushed
the
feature/fix-commands-deploy
branch
from
288c864
to
a077b0d
Compare
commands/service_dev.go
Outdated
| pretty.Progress("Deploying the service...", func() { | ||
| id, valid, err = c.e.ServiceDeploy(c.path) | ||
| }) | ||
| statuses := make(chan provider.DeployStatus, 0) |
There was a problem hiding this comment.
Small change, but if you're putting 0, just skip it and write make(chan provider.DeplyStatus)
warning: should use make(chan provider.DeployStatus) instead (S1019) (megacheck)
commands/service_deploy.go
Outdated
| @@ -4,6 +4,9 @@ import ( | |||
| "errors" | |||
| "fmt" | |||
|
|
|||
| "github.com/mesg-foundation/core/x/xerrors" | |||
|
|
|||
| @@ -23,7 +23,7 @@ type ServiceExecutor interface { | |||
| ServiceByID(id string) (*coreapi.Service, error) | |||
| ServiceDeleteAll() error | |||
| ServiceDelete(ids ...string) error | |||
| ServiceDeploy(path string) (id string, valid bool, err error) | |||
| ServiceDeploy(path string, statuses chan provider.DeployStatus) (id string, validationError, err error) | |||
There was a problem hiding this comment.
Update commands_mock_test.go with new api
commands/service_deploy.go
Outdated
| if !valid { | ||
| return errors.New("Service is invalid. To get more information, run: mesg-core service validate") | ||
| if validationError != nil { | ||
| pretty.DestroySpinner() |
There was a problem hiding this comment.
Here just wonder, shouldn't deployer return DONE DeployedStatus on an error, it's kind of done but with error.
Because Here stoping spinner that hasn't been started in the same function looks suspicious.
The same situation is in service_dev.go
There was a problem hiding this comment.
i had the similar thing in my mind but i wasn't sure to implement this because this is not an exception to validation error. an error may occour at any time during deployment and because of this we may need to destroy spinner unexpectedly. actually, I should move this line to out of validationError's nil check to make sure we call it all the time, thanks for the heads up.
There was a problem hiding this comment.
What about sending DONE to deploy statuses?
There was a problem hiding this comment.
i think errors should be only returned from funcs otherwise we'll have duplication of errors in the place where they're handled. feel free to open an issue about this so we can discuss there.
|
|
||
| const ( | ||
| // RUNNING indicates that status message belongs to an active state. | ||
| RUNNING StatusType = iota + 1 |
There was a problem hiding this comment.
One small change - use iota - there is no obvious reason why counting should be started from 1.
There was a problem hiding this comment.
in go, it's a best practice to count constants from 1. i guess this due to 0 is a zero value in go
There was a problem hiding this comment.
Can you provide some article/best practice about this? I want to read about it.
thanks
There was a problem hiding this comment.
There was a problem hiding this comment.
We could add a status UNKNOWN and start at iota like that it's ok. I agree that we shouldn't start at 0, we can have some side effects with that.
or here
|
|
||
| const ( | ||
| // RUNNING indicates that status message belongs to an active state. | ||
| RUNNING StatusType = iota + 1 |
There was a problem hiding this comment.
We could add a status UNKNOWN and start at iota like that it's ok. I agree that we shouldn't start at 0, we can have some side effects with that.
or here
