You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@apisix.apache.org by GitBox <gi...@apache.org> on 2020/11/05 01:15:14 UTC

[GitHub] [apisix-dashboard] nic-chen opened a new pull request #693: discuss: refactor `conf` of `manager api`

nic-chen opened a new pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693


   relate to  #677
   
   - [ ] Do not write absolute paths in the code
   
   - [ ] Unified format, use snake case for config
   
   


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

[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722138071


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=h1) Report
   > Merging [#693](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=desc) into [v2.0](https://codecov.io/gh/apache/apisix-dashboard/commit/1215c2a8b6c2f1cf6b876feb5d0572a54259d084?el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/693/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##             v2.0     #693      +/-   ##
   ==========================================
   + Coverage   42.82%   42.90%   +0.08%     
   ==========================================
     Files          18       18              
     Lines        1212     1212              
   ==========================================
   + Hits          519      520       +1     
   + Misses        605      604       -1     
     Partials       88       88              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../internal/handler/authentication/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvYXV0aGVudGljYXRpb24vYXV0aGVudGljYXRpb24uZ28=) | `82.35% <100.00%> (ø)` | |
   | [api/internal/handler/route/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGUvcm91dGUuZ28=) | `46.06% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.72% <0.00%> (+0.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=footer). Last update [1215c2a...3c42ded](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517779631



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       `flag.StringVar(&confDir, "c", "./conf/", "conf dir")`
   to
   `flag.StringVar(&confDir, "c", "conf/config.yaml", "config file path")`




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517815549



##########
File path: Makefile
##########
@@ -49,5 +49,5 @@ golang-lint: ## Run the golangci-lint application (install if not found)
 ### api-test:         Run the tests of manager-api
 .PHONY: api-test
 api-test:
-	cd api/ && go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...
+	cd api/ && APISIX_API_WORKDIR=$$PWD go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...

Review comment:
       no, not a file path




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517782727



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       as we talked right now, we should use `flag.StringVar(&workDir, "p", ".", "current work dir")`.
   
   support to specify the prefix path via ENV, `APISIX_API_WORKDIR` seems a good name for us.
   
   we should support both of them.
   
   ```sh
   # style 1
   manager-api -p /home/xxx/apisix/manager-api
   
   # style 2
   APISIX_API_WORKDIR=/home/xxx/apisix/manager-api manager-api
   ```




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r518057056



##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       I copy them from APISIX.

##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       I copy them from APISIX..




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517810120



##########
File path: api/conf/conf.yaml
##########
@@ -0,0 +1,37 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 127.0.0.1

Review comment:
       don't support. but it support `localhost`. I name it from official comment
   
   	// Addr optionally specifies the TCP address for the server to listen on,
   	// in the form "host:port". If empty, ":http" (port 80) is used.
   	// The service names are defined in RFC 6335 and assigned by IANA.
   	// See net.Dial for details of the address format.




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r518028181



##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.
       - 127.0.0.1:2379
 authentication:
-  secret: secret
-  expireTime: 3600
+  secret: secret             # secret for jwt token generation.
+                             # *NOTE*: Highly recommended to modify this value to protect `manager api`.
+                             # if it's default value, when `manager api` start , it will generate a random string to replace it.
+  expireTime: 3600           # jwt token expire time, in second
   users:
-    - username: admin
+    - username: admin        #username and password for login `manager api`

Review comment:
       need a space, `# username ...`

##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       please use better comments, it is not easy to understand

##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -15,20 +15,24 @@
 # limitations under the License.
 #
 
+

Review comment:
       I do not think that is a good way to copy the `api/conf/conf.yaml` again only for testing.
   
   we should use `cp` + `sed` to generate `manager-api-conf.yaml`




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

[GitHub] [apisix-dashboard] juzhiyuan merged pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
juzhiyuan merged pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693


   


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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517861471



##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -0,0 +1,34 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+conf:
+  listen:
+    host: 0.0.0.0
+    port: 8080
+  etcd:
+    endpoints:
+      - 172.16.238.10:2379
+      - 172.16.238.11:2379
+      - 172.16.238.12:2379
+authentication:
+  secret: secret

Review comment:
       comments too




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

[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722138071


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=h1) Report
   > Merging [#693](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=desc) into [v2.0](https://codecov.io/gh/apache/apisix-dashboard/commit/1215c2a8b6c2f1cf6b876feb5d0572a54259d084?el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/693/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##             v2.0     #693      +/-   ##
   ==========================================
   + Coverage   42.82%   42.90%   +0.08%     
   ==========================================
     Files          18       18              
     Lines        1212     1212              
   ==========================================
   + Hits          519      520       +1     
   + Misses        605      604       -1     
     Partials       88       88              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../internal/handler/authentication/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvYXV0aGVudGljYXRpb24vYXV0aGVudGljYXRpb24uZ28=) | `82.35% <100.00%> (ø)` | |
   | [api/internal/handler/route/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGUvcm91dGUuZ28=) | `46.06% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.72% <0.00%> (+0.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=footer). Last update [1215c2a...7e315dd](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517789397



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {

Review comment:
       fixed.




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

[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722138071


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=h1) Report
   > Merging [#693](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=desc) into [v2.0](https://codecov.io/gh/apache/apisix-dashboard/commit/1215c2a8b6c2f1cf6b876feb5d0572a54259d084?el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/693/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##             v2.0     #693      +/-   ##
   ==========================================
   + Coverage   42.82%   42.90%   +0.08%     
   ==========================================
     Files          18       18              
     Lines        1212     1212              
   ==========================================
   + Hits          519      520       +1     
   + Misses        605      604       -1     
     Partials       88       88              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../internal/handler/authentication/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvYXV0aGVudGljYXRpb24vYXV0aGVudGljYXRpb24uZ28=) | `82.35% <100.00%> (ø)` | |
   | [api/internal/handler/route/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGUvcm91dGUuZ28=) | `46.06% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.72% <0.00%> (+0.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=footer). Last update [1215c2a...7e315dd](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [apisix-dashboard] membphis commented on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722218703


   > api/test/docker/manager-api-conf.yaml
   
   use the original `conf/config.yaml` + `sed` is a better way @nic-chen 


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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517857374



##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
 
-services:
-  manager:
-    image: golang:1.13.8
-    volumes:
-      - .:/go/src/github.com/apisix/manager-api
-    working_dir: /go/src/github.com/apisix/manager-api
-    command: go test -v github.com/apisix/manager-api/service
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  etcd:
+    endpoints:
+      - 127.0.0.1:2379
+authentication:
+  secret: secret

Review comment:
       need some comment here




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517782727



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       as we talked right now, we should use `flag.StringVar(&workDir, "p", ".", "current work dir")`.
   
   support to specify the prefix path via ENV, `APISIX_API_WORKDIR` seems a good name for us.
   
   we should support both of them.
   
   ```sh
   # style 1
   manager-api -p /home/xxx/apisix/manager-api
   
   # style 2
   APISIX_API_WORKDIR=/home/xxx/apisix/manager-api manager-api
   ```
   
   @nic-chen what do you think?




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r518092397



##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       it's better.




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517806272



##########
File path: api/Dockerfile
##########
@@ -17,45 +17,34 @@
 
 FROM golang:1.13.8 AS build-env
 
-WORKDIR /go/src/github.com/apisix/manager-api
+RUN mkdir -p /usr/local/apisix-dashboard/api

Review comment:
       ok,let's revert this file




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517804555



##########
File path: Makefile
##########
@@ -49,5 +49,5 @@ golang-lint: ## Run the golangci-lint application (install if not found)
 ### api-test:         Run the tests of manager-api
 .PHONY: api-test
 api-test:
-	cd api/ && go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...
+	cd api/ && APISIX_API_WORKDIR=$$PWD go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...

Review comment:
       that means discover and run any tests within current directly or their subdirectories.




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517859586



##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
 
-services:
-  manager:
-    image: golang:1.13.8
-    volumes:
-      - .:/go/src/github.com/apisix/manager-api
-    working_dir: /go/src/github.com/apisix/manager-api
-    command: go test -v github.com/apisix/manager-api/service
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  etcd:
+    endpoints:
+      - 127.0.0.1:2379
+authentication:
+  secret: secret

Review comment:
       all right




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

[GitHub] [apisix-dashboard] membphis commented on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722361988


   > It's not just used in CI. we run e2e test in local need it too. I think it may cost some time. we could fix it later.
   
   ok, got it. ^_^


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

[GitHub] [apisix-dashboard] membphis commented on pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722122222


   > @membphis
   > 
   > I dealt with the two issues together, because it is too troublesome to debug if divided into two PRs, especially the docker part, the build time is too long
   
   ok, that is fine.
   
   I think we need a better title for this PR. and please take a look at this CI, failed: https://github.com/apache/apisix-dashboard/pull/693/checks?check_run_id=1356022130#step:7:222


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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517830945



##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!

Review comment:
       it makes me confused, please confirm we need those warning message

##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
 
-services:
-  manager:
-    image: golang:1.13.8
-    volumes:
-      - .:/go/src/github.com/apisix/manager-api
-    working_dir: /go/src/github.com/apisix/manager-api
-    command: go test -v github.com/apisix/manager-api/service
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  etcd:
+    endpoints:
+      - 127.0.0.1:2379
+authentication:
+  secret: secret

Review comment:
       why we need this configuration?

##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -0,0 +1,39 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 0.0.0.0
+    port: 8080
+  dag_lib_path: ''

Review comment:
       do we need to delete this line too?




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

[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517929699



##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -15,20 +15,24 @@
 # limitations under the License.
 #
 
+
 conf:
   listen:
-    host: 0.0.0.0
-    port: 8080
+    host: 0.0.0.0            # `manager api` listening ip or host name. It's for e2e test, so it is set to 0.0.0.0
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
-      - 172.16.238.10:2379
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.
+      - 172.16.238.10:2379   # ips here are defined in docker compose.
       - 172.16.238.11:2379
       - 172.16.238.12:2379
 authentication:
-  secret: secret
-  expireTime: 3600
+  secret: secret             # secret for jwt token generation.
+                             # *NOTE*: Highly recommended to modify this value to protect `manager api`.
+                             # if it's default value, when `manager api` start , it will generate a random string to replace it.
+  expireTime: 3600           # jwt token expire time, in second
   users:
-    - username: admin
+    - username: admin        #username and password for login `manager api`

Review comment:
       space after #




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

[GitHub] [apisix-dashboard] nic-chen commented on pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722064518


   @membphis 
   
   I dealt with the two issues together, because it is too troublesome to debug if divided into two PRs, especially the docker part, the build time is too long
   


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

[GitHub] [apisix-dashboard] membphis commented on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722394388


   @nic-chen please confirm we have fixed those points.
   
   ![image](https://user-images.githubusercontent.com/6814606/98249980-de0d0600-1fb1-11eb-9dc0-2a72bd349d11.png)
   


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

[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517786462



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       I would prefer style 1 IMO

##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       I would prefer style 2 IMO




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517796062



##########
File path: api/Dockerfile
##########
@@ -17,45 +17,34 @@
 
 FROM golang:1.13.8 AS build-env
 
-WORKDIR /go/src/github.com/apisix/manager-api
+RUN mkdir -p /usr/local/apisix-dashboard/api

Review comment:
       one PR for one thing, I think we can fix this in new PR

##########
File path: Makefile
##########
@@ -49,5 +49,5 @@ golang-lint: ## Run the golangci-lint application (install if not found)
 ### api-test:         Run the tests of manager-api
 .PHONY: api-test
 api-test:
-	cd api/ && go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...
+	cd api/ && APISIX_API_WORKDIR=$$PWD go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...

Review comment:
       I am confused with `./...`, can you explain this? @nic-chen 

##########
File path: api/internal/handler/route/route.go
##########
@@ -175,7 +175,7 @@ func generateLuaCode(script map[string]interface{}) (string, error) {
 	}
 
 	cmd := exec.Command("sh", "-c",
-		"cd "+conf.DagLibPath+" && lua cli.lua "+
+		"cd "+conf.WorkDir+"/dag-to-lua && lua cli.lua "+

Review comment:
       seems wrong.
   
   we should use `conf.dag_lib_path`, all right?

##########
File path: api/conf/conf.yaml
##########
@@ -0,0 +1,37 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 127.0.0.1

Review comment:
       do we support `*` here?

##########
File path: api/conf/conf.yaml
##########
@@ -0,0 +1,37 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  dag_lib_path: ''

Review comment:
       `''` empty string? is it a valid path?

##########
File path: api/Dockerfile
##########
@@ -17,45 +17,34 @@
 
 FROM golang:1.13.8 AS build-env
 
-WORKDIR /go/src/github.com/apisix/manager-api
+RUN mkdir -p /usr/local/apisix-dashboard/api
+
+WORKDIR /usr/local/apisix-dashboard/api
+
 COPY . .
-RUN mkdir -p /go/manager-api \
-    && mkdir -p /go/manager-api/build-tools \
+
+RUN mkdir -p ../output/conf \
+    && cp ./conf/* ../output/conf
+
+RUN wget https://github.com/api7/dag-to-lua/archive/v1.1.tar.gz -O /tmp/v1.1.tar.gz \

Review comment:
       change it from `/tmp/v1.1.tar.gz` to `/tmp/dag-to-lua.tar.gz`
   




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517777972



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {

Review comment:
       I do not think that is a good way




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r518068890



##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       `supports defining multiple etcd host addresses for an etcd cluster`
   
   how about this?




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

[GitHub] [apisix-dashboard] juzhiyuan commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
juzhiyuan commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517790103



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       OK!




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517809672



##########
File path: api/internal/handler/route/route.go
##########
@@ -175,7 +175,7 @@ func generateLuaCode(script map[string]interface{}) (string, error) {
 	}
 
 	cmd := exec.Command("sh", "-c",
-		"cd "+conf.DagLibPath+" && lua cli.lua "+
+		"cd "+conf.WorkDir+"/dag-to-lua && lua cli.lua "+

Review comment:
       I'will remove that config.users should not need to this dir.




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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517806831



##########
File path: Makefile
##########
@@ -49,5 +49,5 @@ golang-lint: ## Run the golangci-lint application (install if not found)
 ### api-test:         Run the tests of manager-api
 .PHONY: api-test
 api-test:
-	cd api/ && go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...
+	cd api/ && APISIX_API_WORKDIR=$$PWD go test -v -race -cover -coverprofile=coverage.txt -covermode=atomic ./...

Review comment:
       `./...` is it a file path?




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517834348



##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!

Review comment:
       it is useless. i will remove 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

[GitHub] [apisix-dashboard] codecov-io edited a comment on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
codecov-io edited a comment on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722138071


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=h1) Report
   > Merging [#693](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=desc) into [v2.0](https://codecov.io/gh/apache/apisix-dashboard/commit/1215c2a8b6c2f1cf6b876feb5d0572a54259d084?el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/693/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##             v2.0     #693      +/-   ##
   ==========================================
   + Coverage   42.82%   42.90%   +0.08%     
   ==========================================
     Files          18       18              
     Lines        1212     1212              
   ==========================================
   + Hits          519      520       +1     
   + Misses        605      604       -1     
     Partials       88       88              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../internal/handler/authentication/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvYXV0aGVudGljYXRpb24vYXV0aGVudGljYXRpb24uZ28=) | `82.35% <100.00%> (ø)` | |
   | [api/internal/handler/route/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGUvcm91dGUuZ28=) | `46.06% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.72% <0.00%> (+0.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=footer). Last update [1215c2a...87b06b9](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [apisix-dashboard] nic-chen commented on pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722249435


   > use the original `conf/config.yaml` + `sed`
   
   It's not just used in CI. we run e2e test in local need it too. I think it may cost some time. we could fix it later.


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

[GitHub] [apisix-dashboard] membphis commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
membphis commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r518066534



##########
File path: api/conf/conf.yaml
##########
@@ -17,16 +17,18 @@
 
 conf:
   listen:
-    host: 127.0.0.1
-    port: 8080
+    host: 127.0.0.1          # `manager api` listening ip or host name
+    port: 8080               # `manager api` listening port
   etcd:
-    endpoints:
+    endpoints:               # it's possible to define multiple etcd hosts addresses of the same etcd cluster.

Review comment:
       we need to update the APISIX too ^_^




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517899953



##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -0,0 +1,34 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+
+conf:
+  listen:
+    host: 0.0.0.0
+    port: 8080
+  etcd:
+    endpoints:
+      - 172.16.238.10:2379
+      - 172.16.238.11:2379
+      - 172.16.238.12:2379
+authentication:
+  secret: secret

Review comment:
       done.




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517787238



##########
File path: api/conf/conf.go
##########
@@ -79,12 +82,19 @@ type Config struct {
 }
 
 func init() {
-	flag.StringVar(&confDir, "c", "./conf/", "conf dir")
-	flag.Parse()
+	//go test
+	if strings.HasSuffix(os.Args[0], ".test") {
+		_, basePath, _, _ := runtime.Caller(0)
+		confDir = filepath.Dir(basePath) + "/"
+		fmt.Println("confDir:", confDir)
+	} else {
+		flag.StringVar(&confDir, "c", "./conf/", "conf dir")

Review comment:
       it's better. and user don't need to config other dirs or file paths.




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517808905



##########
File path: api/conf/conf.yaml
##########
@@ -0,0 +1,37 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  dag_lib_path: ''

Review comment:
       it should be removed.




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

[GitHub] [apisix-dashboard] codecov-io commented on pull request #693: discuss: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
codecov-io commented on pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#issuecomment-722138071


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=h1) Report
   > Merging [#693](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=desc) into [v2.0](https://codecov.io/gh/apache/apisix-dashboard/commit/1215c2a8b6c2f1cf6b876feb5d0572a54259d084?el=desc) will **increase** coverage by `0.08%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/693/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##             v2.0     #693      +/-   ##
   ==========================================
   + Coverage   42.82%   42.90%   +0.08%     
   ==========================================
     Files          18       18              
     Lines        1212     1212              
   ==========================================
   + Hits          519      520       +1     
   + Misses        605      604       -1     
     Partials       88       88              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [.../internal/handler/authentication/authentication.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvYXV0aGVudGljYXRpb24vYXV0aGVudGljYXRpb24uZ28=) | `82.35% <100.00%> (ø)` | |
   | [api/internal/handler/route/route.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvcm91dGUvcm91dGUuZ28=) | `46.06% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/693/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.72% <0.00%> (+0.70%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=continue).
   > **Legend** - [Click here to learn more](https://docs.codecov.io/docs/codecov-delta)
   > `Δ = absolute <relative> (impact)`, `ø = not affected`, `? = missing data`
   > Powered by [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=footer). Last update [1215c2a...e7eaad9](https://codecov.io/gh/apache/apisix-dashboard/pull/693?src=pr&el=lastupdated). Read the [comment docs](https://docs.codecov.io/docs/pull-request-comments).
   


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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517834567



##########
File path: api/test/docker/manager-api-conf.yaml
##########
@@ -0,0 +1,39 @@
+#
+# Licensed to the Apache Software Foundation (ASF) under one or more
+# contributor license agreements.  See the NOTICE file distributed with
+# this work for additional information regarding copyright ownership.
+# The ASF licenses this file to You under the Apache License, Version 2.0
+# (the "License"); you may not use this file except in compliance with
+# the License.  You may obtain a copy of the License at
+#
+#     http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+#
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
+
+conf:
+  listen:
+    host: 0.0.0.0
+    port: 8080
+  dag_lib_path: ''

Review comment:
       sure.




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

[GitHub] [apisix-dashboard] nic-chen commented on a change in pull request #693: chore: refactor `conf` of `manager api`

Posted by GitBox <gi...@apache.org>.
nic-chen commented on a change in pull request #693:
URL: https://github.com/apache/apisix-dashboard/pull/693#discussion_r517834801



##########
File path: api/conf/conf.yaml
##########
@@ -14,12 +14,23 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 #
-version: '3'
+# PLEASE DO NOT UPDATE THIS FILE!
+# If you want to set the specified configuration value, you can set the new
+# value in the conf/config.yaml file.
+#
 
-services:
-  manager:
-    image: golang:1.13.8
-    volumes:
-      - .:/go/src/github.com/apisix/manager-api
-    working_dir: /go/src/github.com/apisix/manager-api
-    command: go test -v github.com/apisix/manager-api/service
+conf:
+  listen:
+    host: 127.0.0.1
+    port: 8080
+  etcd:
+    endpoints:
+      - 127.0.0.1:2379
+authentication:
+  secret: secret

Review comment:
       it's for jwt token generation




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