[GitHub] [incubator-yunikorn-core] kingamarton commented on a change in pull request #231: YUNIKORN-418: Add "config" REST API

[GitBox <gi...@apache.org>]

kingamarton commented on a change in pull request #231:
URL: https://github.com/apache/incubator-yunikorn-core/pull/231#discussion_r563584769



##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)

Review comment:
       Pleas include in the error message exactly what parameter is missing. In our case "dry_run"

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]

Review comment:
       Please don't ignore the returned value here and reuse it in the line 469:
   
   ```
   dryRun, dryRunExists := queryParams["dry_run"]
   ....
   if dryRun != "1" {
   ...
   ```

##########
File path: pkg/webservice/routes.go
##########
@@ -102,7 +102,15 @@ var webRoutes = routes{
 		"Scheduler",
 		"PUT",
 		"/ws/v1/config",
-		updateConfig,
+		updateClusterConfig,
+	},
+
+	// endpoint to update the current conf

Review comment:
       this is the endpoint to validate the config, not? the update is the previous one.

##########
File path: pkg/webservice/webservice.go
##########
@@ -58,14 +59,40 @@ func newRouter() *mux.Router {
 func loggingHandler(inner http.Handler, name string) http.Handler {
 	return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
 		start := time.Now()
-
-		inner.ServeHTTP(w, r)
-
+		rw := &YResponseWriter{ResponseWriter: w, statusCode: -1}
+		inner.ServeHTTP(rw, r)
+		var body = string(rw.body)
+		if rw.statusCode != 200 && rw.statusCode != -1 {
+			errorInfo := dao.NewYAPIError(nil, rw.statusCode, body)

Review comment:
       This `NewYAPIError` method is not defined. This is the reason why Travis check is failing. 

##########
File path: pkg/webservice/handlers.go
##########
@@ -457,7 +457,36 @@ func getClusterConfig(w http.ResponseWriter, r *http.Request) {
 	}
 }
 
-func updateConfig(w http.ResponseWriter, r *http.Request) {
+func createClusterConfig(w http.ResponseWriter, r *http.Request) {
+	writeHeaders(w)
+
+	queryParams := r.URL.Query()
+	_, dryRunExists := queryParams["dry_run"]
+	if len(queryParams) == 0 || !dryRunExists {
+		http.Error(w, "Mandatory parameters are missing in URL path. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}
+	if queryParams.Get("dry_run") != "1" {
+		http.Error(w, "Invalid query params. Please check the usage documentation", http.StatusBadRequest)
+		return
+	}

Review comment:
       Right now we support only dry_run=1. I think we should document it and also include it in the error message.




----------------------------------------------------------------
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org