[GitHub] [apisix-dashboard] starsz commented on a change in pull request #1788: feat: embed assets in api binary

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

starsz commented on a change in pull request #1788:
URL: https://github.com/apache/apisix-dashboard/pull/1788#discussion_r621747645



##########
File path: Makefile
##########
@@ -50,10 +59,17 @@ endif
 .PHONY: api-default
 api-default:
 ifeq ("$(wildcard $(GO_EXEC))", "")
-	@echo "ERROR: Need to install golang 1.13+ first"
+	@echo "ERROR: Need to install golang 1.16+ first"
 	exit 1
 endif
-
+	@if [ "${GO_MAJOR}" -lt "${REQUIRED_MIN_GO_MAJOR}" ] ; then \
+		echo "ERROR: Need to install golang 1.16+ first"; \
+		exit 1; \
+	fi
+	@if [ "${GO_MINOR}" -lt "${REQUIRED_MIN_GO_MINOR}" ] ; then \
+		echo "ERROR: Need to install golang 1.16+ first"; \
+		exit 1; \
+	fi

Review comment:
       The logic will make a mistake if we use the go version like 2.0 in the future.
   : )

##########
File path: api/internal/route.go
##########
@@ -54,10 +55,13 @@ func SetUpRouter() *gin.Engine {
 	r := gin.New()
 	logger := log.GetLogger(log.AccessLog)
 	r.Use(filter.CORS(), filter.RequestId(), filter.IPFilter(), filter.RequestLogHandler(logger), filter.SchemaCheck(), filter.RecoverHandler())
-	r.Use(static.Serve("/", static.LocalFile(filepath.Join(conf.WorkDir, conf.WebDir), false)))
-	r.NoRoute(func(c *gin.Context) {
-		c.File(fmt.Sprintf("%s/index.html", filepath.Join(conf.WorkDir, conf.WebDir)))
-	})
+	filesystem := fs.FS(StaticFiles)
+	subtree, err := fs.Sub(filesystem, "html")
+

Review comment:
       Redundant line.

##########
File path: api/internal/route.go
##########
@@ -54,10 +55,13 @@ func SetUpRouter() *gin.Engine {
 	r := gin.New()
 	logger := log.GetLogger(log.AccessLog)
 	r.Use(filter.CORS(), filter.RequestId(), filter.IPFilter(), filter.RequestLogHandler(logger), filter.SchemaCheck(), filter.RecoverHandler())
-	r.Use(static.Serve("/", static.LocalFile(filepath.Join(conf.WorkDir, conf.WebDir), false)))
-	r.NoRoute(func(c *gin.Context) {
-		c.File(fmt.Sprintf("%s/index.html", filepath.Join(conf.WorkDir, conf.WebDir)))
-	})
+	filesystem := fs.FS(StaticFiles)
+	subtree, err := fs.Sub(filesystem, "html")
+
+	if err != nil {
+		log.Errorf("%s\n", err)

Review comment:
       I think we should panic here if we can't embed assets in API binary.

##########
File path: api/test/shell/cli_test.sh
##########
@@ -118,45 +118,45 @@ if [[ `grep -c "INFO" ./error.log` -eq '0' ]]; then
 fi
 
 # run on a different path
-workDir=$(pwd)
-rm -rf html
-mkdir html
-cd html
-echo "hi~" >> index.html
-APISIX_API_WORKDIR=$workDir $workDir/manager-api &
-sleep 5
-
-res=$(curl http://127.0.0.1:9000)
-$workDir/manager-api stop
-sleep 6
-cd -
-rm -rf html
-
-if [[ $res != "hi~" ]]; then
-    echo "failed: manager-api cant run on a different path"
-    exit 1
-fi
+# workDir=$(pwd)

Review comment:
       I think we can remove them if we don't use them.




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