Show correct message on NodeStatus failure #564
Conversation
HomayoonAlimohammadi
force-pushed
the
KU-1096/separate-errors-for-node-status-request
branch
from
e29b359
to
91d1e7a
Compare
HomayoonAlimohammadi
changed the title
src/k8s/cmd/k8s/node_status.go
Outdated
| "github.com/canonical/lxd/shared/api" | ||
| ) | ||
|
|
||
| // GetNodeStatus retrieves the NodeStatus from k8sd client. |
There was a problem hiding this comment.
nit: it retrieves it from the server - the client just does the underlying http request.
Also, let's not overdo it with the line breaks. The comments can be at least the same length as the code.
src/k8s/cmd/k8s/k8s_config.go
Outdated
| env.Exit(1) | ||
| return | ||
| } | ||
| _ = GetNodeStatus(client, cmd, env) |
There was a problem hiding this comment.
This implementation hides a lot of details and is not particular easy to read.
You would need to check the implementation of GetNodeStatus to understand what is going on and why we ignore everything here.
I would:
a) change the GetNodeStatus function to return the status and an error
b) create a new function e.g. isBootstrapped that wraps around the GetNodeStatus call, handles the error decoding logic and returns a bool
c) Then you can do something like this in the commands:
if isBootstrapped(...) {
cmd.Print("Node already bootstrapped/part of a cluster etc")
cmd.Exit(1)
}
There was a problem hiding this comment.
thanks for the amazing comment @bschimke95 . I tried to address your concern. it didn't turn out to be exactly like you suggested but I guess it's close. returning 3 things does bother me a bit but I feel it's not really that bad. it reduces duplication, it's easily testable and more readable than the previous version. the name isBootstrapped does not seem to work here since we need to return the status and an error as well.
src/k8s/cmd/k8s/node_status.go
Outdated
There was a problem hiding this comment.
the files in src/k8s/cmd/k8s follow the format k8s_<command>_<subcommand>.go
That way a developer can easily see what commands the CLI supports.
Therefore, please move utils like that to src/k8s/cmd/utils.
.gitignore
Outdated
| @@ -1,6 +1,8 @@ | |||
| /parts/ | |||
| /stage/ | |||
| /prime/ | |||
| .vscode/ | |||
There was a problem hiding this comment.
don't folks normally put this in their global gitignore?
https://blog.martinhujer.cz/dont-put-idea-vscode-directories-to-projects-gitignore/
I don't really know how i land on this...
There was a problem hiding this comment.
TIL about global git ignore 😄
.gitignore
Outdated
| /docs/tools/.sphinx/node_modules No newline at end of file | ||
| /docs/tools/.sphinx/node_modules |
There was a problem hiding this comment.
so you've added a newline here -- i like files with newlines at the end
thanks for having that here...
HomayoonAlimohammadi
deleted the
KU-1096/separate-errors-for-node-status-request
branch
* Show correct message on NodeStatus failure
Make sure we don't show the
you don't have a cluster, run k8s bootstrapin wrong scenarios.