Adjust the cli directory structure.Fixed some bug. #168
Conversation
client/client.go
Outdated
| if c.Endpoint == "" { | ||
| c.Endpoint = os.Getenv("OPENSDS_ENDPOINT") |
There was a problem hiding this comment.
I think we can't directly set the default endpoint like this, maybe it's a better idea to remind user to import OPENSDS_ENDPOINT first.
There was a problem hiding this comment.
Using the ENV to set the endpoint is ok, but one user want use the client must set the ENV first everytime, I think it`s not very friendly. And user usually use the client in the local host, I think is if one user does not specfied the OPENSDS_ENDPOINT , using the the localhost as its default value maybe better.
There was a problem hiding this comment.
I know what you said, what I mean is maybe a better way is to pull an error if user doesn't specify the ENV. One use case is that for a new user, if he doesn't specify the endpoint ENV, you solution will assign 127.0.0.1:50040 to the variable, but if something goes wrong, user would be confused about the reported error since he didn't notice that he needs to configure the endpoint.
What do you think?
There was a problem hiding this comment.
Maybe we can print in log file to notice user, or get the endpoint infoamtion from config file.
There was a problem hiding this comment.
What about storing the default endpoint of both config and client in a unified file, and the client will be set to 127.0.0.1:50040 default?
| @@ -24,12 +24,19 @@ import ( | |||
| "log" | |||
| "os" | |||
|
|
|||
| "github.com/opensds/opensds/pkg/cli" | |||
| "github.com/opensds/opensds/osdsctl" | |||
There was a problem hiding this comment.
Sorry I didn't get it what's the difference between these two package path?
There was a problem hiding this comment.
The CLI will invoke the CLIENT packages, but the directory hierarchy of CLIENT packges is higher than the CLI`s. And this makes a little confusing for developers.
There was a problem hiding this comment.
Ok, in this way, I suggest you could have a look at etcd and see what we can learn from it.
| @@ -177,3 +178,14 @@ func MergeStringMaps(maps ...map[string]string) map[string]string { | |||
| } | |||
| return out | |||
| } | |||
|
|
|||
| func PathExists(path string) (bool, error) { | |||
There was a problem hiding this comment.
Can you add unit test of this function? And one thing need to mention is that I guess you didn't run go fmt ..., please have a check at it.
|
good! |
This pr fixed some bugs, and modfied the cli directory structure.