pbacsko commented on code in PR #750:
URL: https://github.com/apache/yunikorn-core/pull/750#discussion_r1439106714
##########
pkg/webservice/handlers.go:
##########
@@ -58,8 +58,43 @@ const (
GroupNameMissing = "Group name is missing"
ApplicationDoesNotExists = "Application not found"
NodeDoesNotExists = "Node not found"
+ ActiveState = "active"
)
+var allowedStatesMsg string
+var allowedActiveStatesMsg string
+var allowedAppStates map[string]bool
+var allowedAppActiveStates map[string]bool
+
+func init() {
+ allowedAppStates = make(map[string]bool)
+ allowedAppActiveStates = make(map[string]bool)
+
+ allowedAppStates[ActiveState] = true
+ allowedAppStates[strings.ToLower(objects.Rejected.String())] = true
+ allowedAppStates[strings.ToLower(objects.Completed.String())] = true
+
+ var states []string
+ for k := range allowedAppStates {
+ states = append(states, k)
+ }
+ allowedStatesMsg = fmt.Sprintf("Only following application states are
allowed: %s", strings.Join(states, ","))
+
+ allowedAppActiveStates[strings.ToLower(objects.New.String())] = true
+ allowedAppActiveStates[strings.ToLower(objects.Accepted.String())] =
true
+ allowedAppActiveStates[strings.ToLower(objects.Starting.String())] =
true
+ allowedAppActiveStates[strings.ToLower(objects.Running.String())] = true
+ allowedAppActiveStates[strings.ToLower(objects.Completing.String())] =
true
+ allowedAppActiveStates[strings.ToLower(objects.Failing.String())] = true
+ allowedAppActiveStates[strings.ToLower(objects.Resuming.String())] =
true
Review Comment:
We might need a better naming here. It's a bit difficult to distingush
between `allowedAppStates` and `allowedAppActiveStates`. At least some comments:
```
var allowedStatesMsg string // returned error message when the requested
application state is invalid
var allowedActiveStatesMsg string // returned error message when the actual
state of the application is not an active state like Running, Accepted, etc.
var allowedAppStates map[string]bool // ??? (this one is a bit tough to
explain, we mix two existing states with a higher level one which)
var allowedAppActiveStates map[string]bool // list of application states
that are valid for filtering
```
We can actually just drop `allowedAppStates` because it only contains 3
values. Readability is important, so this is better:
```
if appState != "active" && appState != "rejected" && appState !=
"completed" {
buildJSONErrorResponse(w, "Only following application states
are allowed: active, rejected, completed", http.StatusBadRequest)
return
}
```
You can also just drop the `strings.ToLower()` calls, it makes the code a
bit clumsy. Just write:
```
allowedAppActiveStates["new"] = true
allowedAppActiveStates["accepted"] = true
allowedAppActiveStates["starting"] = true
...
```
Obviously this is hard-coded and we no longer reference the code which
define the states, but something for something. However, it's pretty unlikely
that the state machine will ever change. You can add a comment to
`application_state.go` above the `const` section:
`// Application states are used for filtering in the webservice handlers.
Please check&update the logic if needed if the state machine is modified`
Sometimes we need to be pragmatic.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
