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/28 10:54:24 UTC

[GitHub] [apisix-dashboard] starsz opened a new pull request #900: feat: add version info into manager-api 

starsz opened a new pull request #900:
URL: https://github.com/apache/apisix-dashboard/pull/900


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [ ] Bugfix
   - [x] New feature provided
   - [ ] Improve performance
   
   - Related issues
   
   fix #866
   fix #853 


----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   ping @starsz @ShiningRush 


----------------------------------------------------------------
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] starsz commented on pull request #900: feat: add version info into manager-api

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


   > missing test case
   
   added.


----------------------------------------------------------------
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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Version", Version)
+	fmt.Fprintf(os.Stdout, "%-8s: %s:%d\n", "Listen", conf.ServerHost, conf.ServerPort)
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Loglevel", conf.ErrorLogLevel)

Review comment:
       The listen info is also described in the config file. But we show it.
   So I think it's more convenient for users to get the log file path location if the file path is described in `relative path` 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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Version", Version)
+	fmt.Fprintf(os.Stdout, "%-8s: %s:%d\n", "Listen", conf.ServerHost, conf.ServerPort)
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Loglevel", conf.ErrorLogLevel)

Review comment:
       Output listen info have two meanings:
   - Tell user, manager api is running normal
   - Show the serve info
   
   If we output the log info, why we not output other config? We should give a standard to tell other developer what should be print with begin info instead of component's log,I  think `relative path` is not reasonable.




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       This is a different line with `listen on:`, this line dose not offer any helpful info to user.
   The original issue demo is just only one line `manager-api is running, and listen on: http://127.0.0.1:8080.`
   > And -v will be implemented at #773. Manager-api need a cli scafford.
   
   Got it, but I think it would be better have a discuss before coding, here are many projects implement the command just using native go `flag` library, are we really need that?  
   
   BTW: I think `manager-api is listen on : xxxx`  would be more simple,how do you think? @membphis @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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       I agree with add go version. But why are branch name and build date useful, since we had git commit info.
   
   In fact, I refer the [mosn](https://github.com/mosn/mosn/blob/2adbcbc57994288fa4bdeb86bf43973443233a0b/Makefile#L94)




----------------------------------------------------------------
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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       Got it. BTW, why we need to add the branch name?




----------------------------------------------------------------
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 merged pull request #900: feat: add version info into manager-api

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


   


----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=h1) Report
   > Merging [#900](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=desc) (f2f448a) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/4196844e033800754640351aec096ce0eac5573e?el=desc) (4196844) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/900/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=tree)
   
   ```diff
   @@           Coverage Diff           @@
   ##           master     #900   +/-   ##
   =======================================
     Coverage   41.80%   41.80%           
   =======================================
     Files          23       23           
     Lines        1428     1428           
   =======================================
     Hits          597      597           
     Misses        740      740           
     Partials       91       91           
   ```
   
   
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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/900?src=pr&el=footer). Last update [4196844...f2f448a](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       We also need go version, it is useful to debug problem, and if we add the git version, it would be better to add branch name. and build date is also useful.
   You can refer [here](https://github.com/m3db/m3/blob/master/scripts/go-build-ldflags.sh).




----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=h1) Report
   > Merging [#900](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=desc) (01e35d0) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/277ae594b213c1c0bfb9aa0d387622cc9c65a6cc?el=desc) (277ae59) will **not change** coverage.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/900/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=tree)
   
   ```diff
   @@           Coverage Diff           @@
   ##           master     #900   +/-   ##
   =======================================
     Coverage   43.03%   43.03%           
   =======================================
     Files          18       18           
     Lines        1271     1271           
   =======================================
     Hits          547      547           
     Misses        632      632           
     Partials       92       92           
   ```
   
   
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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/900?src=pr&el=footer). Last update [277ae59...01e35d0](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       > If here are many branch, how do you know to which branch the git version belong?
   
   For this, help us to locate at which branch git version is




----------------------------------------------------------------
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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       Sorry for the PR title. It's not only the version info. It also fixes #853.
   And `-v` will be implemented at https://github.com/apache/apisix-dashboard/pull/773. Manager-api need a cli scafford.




----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   @starsz I helped you resolve the conflict


----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=h1) Report
   > Merging [#900](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=desc) (741d346) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/277ae594b213c1c0bfb9aa0d387622cc9c65a6cc?el=desc) (277ae59) will **decrease** coverage by `0.07%`.
   > The diff coverage is `n/a`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/900/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master     #900      +/-   ##
   ==========================================
   - Coverage   43.03%   42.95%   -0.08%     
   ==========================================
     Files          18       18              
     Lines        1271     1271              
   ==========================================
   - Hits          547      546       -1     
   - Misses        632      633       +1     
     Partials       92       92              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/900?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/900/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.28% <0.00%> (-0.66%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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/900?src=pr&el=footer). Last update [277ae59...741d346](https://codecov.io/gh/apache/apisix-dashboard/pull/900?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] juzhiyuan commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/VERSION
##########
@@ -0,0 +1 @@
+v2.1-rc1

Review comment:
       Got it, we may have this tip to release guide 😃 




----------------------------------------------------------------
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 #900: feat: add version info into manager-api

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


   @starsz many thx, merged right now


----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       This is a different line with `listen on:`, this line dose not offer any helpful info to user.
   The original issues demo is just only one line `manager-api is running, and listen on: http://127.0.0.1:8080.`
   BTW: I think `manager-api is listen on : xxxx`  would be more simple,how do you think? @membphis @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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       If here are many branch, how do you know to which branch the git version belong?




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       If here are many branch, how do you know to which branch the git version belong?
   Build date can help to check the binary is build by ours or user.




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Version", Version)
+	fmt.Fprintf(os.Stdout, "%-8s: %s:%d\n", "Listen", conf.ServerHost, conf.ServerPort)
+	fmt.Fprintf(os.Stdout, "%-8s: %s\n", "Loglevel", conf.ErrorLogLevel)

Review comment:
       Log info is described in config file, I think here show listen info is enough because this is the most important running info to user, how 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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/VERSION
##########
@@ -0,0 +1 @@
+v2.1-rc1

Review comment:
       Yeah. It's important. I had checked the apisix project. It needs to update `version.lua` for every release.
   The same as apisix-dashboard. 
   
   Maybe we can add a checklist to the PR template?




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       Just keep version info short, welcome is unnecessary.And it is better to implement a `-v` command for 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] juzhiyuan commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/VERSION
##########
@@ -0,0 +1 @@
+v2.1-rc1

Review comment:
       🤔 Do we have to modify this file before releasing it? The release manager may forget this point sometimes. We should update docs or add some tips somewhere 😂

##########
File path: api/VERSION
##########
@@ -0,0 +1 @@
+v2.1-rc1

Review comment:
       Do we have to modify this file before releasing it? The release manager may forget this point sometimes. We should update docs or add some tips somewhere 🤔




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       Code is same is not enough, user could build it with different build tag and option(such as enable static link and disable cgo etc.).




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       This is a different line with `listen on:`, this line dose not offer any helpful info to user.
   The original issue demo is just only one line `manager-api is running, and listen on: http://127.0.0.1:8080.`
   > Manager-api need a cli scafford.
   
   got it.
   
   BTW: I think `manager-api is listen on : xxxx`  would be more simple,how do you think? @membphis @nic-chen 

##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       This is a different line with `listen on:`, this line dose not offer any helpful info to user.
   The original issue demo is just only one line `manager-api is running, and listen on: http://127.0.0.1:8080.`
   > And -v will be implemented at #773. Manager-api need a cli scafford.
   
   got it.
   
   BTW: I think `manager-api is listen on : xxxx`  would be more simple,how do you think? @membphis @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] juzhiyuan commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/VERSION
##########
@@ -0,0 +1 @@
+v2.1-rc1

Review comment:
       Do we have to modify this file before releasing it? The release manager may forget this point sometimes. We should update docs or add some tips somewhere if we use this file to save version. 🤔




----------------------------------------------------------------
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] ShiningRush commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/main.go
##########
@@ -35,6 +35,16 @@ import (
 	"github.com/apisix/manager-api/log"
 )
 
+var Version string
+
+func printInfo() {
+	fmt.Fprint(os.Stdout, "The manager-api is running successfully!\n\n")

Review comment:
       Just keep version info short, welcome is unnecessary




----------------------------------------------------------------
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] starsz commented on a change in pull request #900: feat: add version info into manager-api

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



##########
File path: api/build.sh
##########
@@ -19,6 +19,9 @@ set -ex
 export ENV=local
 pwd=`pwd`
 
+VERSION=$(cat ./api/VERSION)

Review comment:
       Hi, I write the `abbreviated commit hash`.
   If the commit hash is the same, we can consider the code is the same.
   




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