ciao-controller: check instances in list belong to tenant #318
Conversation
| instance, err := context.ds.GetInstance(instanceID) | ||
|
|
||
| if err != nil { | ||
| instanceError := fmt.Sprintf("Instance %s could not be found", instanceID) |
There was a problem hiding this comment.
@leoswaldo Could you please make returnErrorCode() a variadic function to avoid explicitly building these strings ?
The rest if the code looks fine to me.
leoswaldo
force-pushed
the
leoswaldo/issue/98
branch
from
dc1d46f to
cb7a375
Compare
| var returnCode payloads.HTTPReturnErrorCode | ||
| returnCode.Error.Code = httpError | ||
| returnCode.Error.Name = http.StatusText(returnCode.Error.Code) | ||
| returnCode.Error.Message = message | ||
|
|
||
| returnCode.Error.Message = strings.Join(message, " ") |
There was a problem hiding this comment.
Sorry if my initial comment was confusing, but this is what I had in mind:
diff --git a/ciao-controller/compute.go b/ciao-controller/compute.go
index 75f5153..2ed9ce5 100644
--- a/ciao-controller/compute.go
+++ b/ciao-controller/compute.go
@@ -453,11 +453,11 @@ func instanceToServer(context *controller, instance *types.Instance) (payloads.S
}
// returnErrorCode returns error codes for the http call
-func returnErrorCode(w http.ResponseWriter, httpError int, message string) {
+func returnErrorCode(w http.ResponseWriter, httpError int, format string, args ...interface{}) {
var returnCode payloads.HTTPReturnErrorCode
returnCode.Error.Code = httpError
returnCode.Error.Name = http.StatusText(returnCode.Error.Code)
- returnCode.Error.Message = message
+ returnCode.Error.Message = fmt.Sprintf(format, args)
b, err := json.Marshal(returnCode)
if err != nil {
And then you can write code like:
returnErrorCode(w, http.StatusNotFound, "Instance %d could not be found", instanceID)
14e38a2 to
eb8ecce
Compare
|
@leoswaldo Could you rebase this one against master? The original commit is quite old now. |
|
Apart from that it looks okay to me. |
|
It would also be awesome if you added some test coverage on ciao-controller/compute.go functions while you're at it. The tenantServersAction() function has quite some gaps according to coveralls, eg: https://coveralls.io/builds/6967637/source?filename=ciao-controller%2Fcompute.go |
returnErrorCode function receives a message string which it will return with this design the message needed to be formatted before calling the function, making the function variadic for the message will enable the function to be call with the messages to be formatted in the function, so we don't have to preformat every time we call it. The returnErrorCode func will now build the message string in the format specified with the args passed to it as values. Signed-off-by: Leoswaldo Macias <[email protected]>
tenantServerAction function was acting on instances when a list of servers was provided, the function now validates that if a servers list is provided then ensure it belongs to the tenant specified before proceeding to the action. Fixes ciao-project#98 Signed-off-by: Leoswaldo Macias <[email protected]>
leoswaldo
force-pushed
the
leoswaldo/issue/98
branch
from
eb8ecce to
2b4f857
Compare
|
@markdryan rebased As @tpepper mentioned, with last integrations to compute.go it has decreased the code coverage, right now I'm facing issues to set travis env setup in CLR (my development is on ClearLinux), so to avoid making longer this pull request that already has a fix, I'm updating it and opening a new issue for test coverage #349 to handle testing in a different thread, and not only for this change. |
|
fyi @leoswaldo you shouldn't need to do anything with Travis. You can cd ciao-controller and then "go test -v -short" to run the tests locally in a way comparable to what Travis does in the cloud. Add "-cover" to see a coverage summary or "-coverprofile=somefile" and then "go tool cover -html=somefile" will pop open a web browser on you dev box with an code navigator. You can do this easily on fedora or Ubuntu or Mac or even Windows I assume...or the full featured development platform of your choice. It's probably hardest on clearlinux. You can do this all locally on a laptop or whatever with just go tools (and optionally an html viewer). |
|
@markdryan Yes, this is fine with me. |
|
Ok with me as well. |
tenantServerAction function was acting on instances when a list of
servers was provided, the function now validates that if a servers
list is provided then ensure it belongs to the tenant specified
before proceeding to the action.
Fixes #98
Signed-off-by: Leoswaldo Macias [email protected]