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/12/17 15:51:10 UTC

[GitHub] [apisix-dashboard] nic-chen opened a new pull request #1064: fix: when enable or disable existing SSL, an error occurred

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


   Please answer these questions before submitting a pull request
   
   - Why submit this pull request?
   - [x] Bugfix
   - [ ] New feature provided
   - [ ] Improve performance
   
   - Related issues
   fix #1034
   
   ___
   ### Bugfix
   - Description
   The SSL `GET` api does not return the SSL key, so set `ssl.key` to nil, which will be saved in the cache. 
   At this time, use the patch method to update the SSL, because the lack of the key, it fails the schema verification, and return an error.
   The `ErrorWrapper` use it directly without judging the type of error, which eventually leads to panic.
   
   - How to fix?
   
   Set the key to nil after clone `ssl` to avoid affecting the data in the cache.
   After judging the error type, do the corresponding treatment before use.
   
   


----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (43fa765) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/d2b577298d4b86ea120fda718e21478cd94e82eb?el=desc) (d2b5772) will **increase** coverage by `0.72%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   41.40%   42.12%   +0.72%     
   ==========================================
     Files          28       29       +1     
     Lines        1785     1816      +31     
   ==========================================
   + Hits          739      765      +26     
   - Misses        940      943       +3     
   - Partials      106      108       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.14% <60.00%> (+0.24%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [d2b5772...43fa765](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/utils/consts/api_error.go
##########
@@ -27,7 +29,21 @@ func ErrorWrapper(handle WrapperHandle) gin.HandlerFunc {
 	return func(c *gin.Context) {
 		data, err := handle(c)
 		if err != nil {
-			apiError := err.(*ApiError)
+			apiError, ok := err.(*ApiError)
+			if !ok {
+				errMsg := err.Error()
+				if strings.Contains(errMsg, "required") ||

Review comment:
       @starsz  sure, we need a PR to optimize 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] codecov-io edited a comment on pull request #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (5372e80) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/c3e1d162bbaf19b1cf63c0437b3bec874c988dd5?el=desc) (c3e1d16) will **increase** coverage by `0.79%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   41.00%   41.79%   +0.79%     
   ==========================================
     Files          28       29       +1     
     Lines        1773     1804      +31     
   ==========================================
   + Hits          727      754      +27     
   - Misses        940      942       +2     
   - Partials      106      108       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.14% <60.00%> (+0.24%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `79.22% <0.00%> (+0.64%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [c3e1d16...5372e80](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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 merged pull request #1064: fix: when enable or disable existing SSL, an error occurred

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


   


----------------------------------------------------------------
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] spacewander commented on a change in pull request #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       I remember the literal struct initialization is encouraged...
   https://github.com/uber-go/guide/blob/master/style.md#initializing-struct-references




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (91bcec0) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/303cc5612feac82dc27305f1c29762262693eda5?el=desc) (303cc56) will **increase** coverage by `0.78%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   40.13%   40.92%   +0.78%     
   ==========================================
     Files          26       27       +1     
     Lines        1572     1603      +31     
   ==========================================
   + Hits          631      656      +25     
   - Misses        850      854       +4     
   - Partials       91       93       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `52.50% <60.00%> (+1.07%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.57% <0.00%> (-0.65%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [303cc56...2088687](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   > @nic-chen need E2E test cases to confirm this bug ^_^
   
   @membphis 
   we already have E2E test cases  for it:
   https://github.com/nic-chen/incubator-apisix-dashboard/blob/fix-ssl-status-bug/api/test/e2e/ssl_test.go#L77-L132
   
   just need to add a test case to trigger the bug:
   https://github.com/apache/apisix-dashboard/pull/1064/files#diff-c2b7edfd95881ffeb7a02870b56d8c228b7b1f0f83d78be56f4462091bf76dc7R104-R111
   
   


----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (c563c21) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/46bf1ef9cfe4f4c659752176424151bb4fc4952f?el=desc) (46bf1ef) will **increase** coverage by `0.73%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   41.06%   41.79%   +0.73%     
   ==========================================
     Files          28       29       +1     
     Lines        1773     1804      +31     
   ==========================================
   + Hits          728      754      +26     
   - Misses        939      942       +3     
   - Partials      106      108       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.14% <60.00%> (+0.24%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [46bf1ef...c563c21](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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] codecov-io edited a comment on pull request #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (487c345) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/97c53973ffc84e488add40ac3fe139ba3dc43ba7?el=desc) (97c5397) will **increase** coverage by `0.73%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   41.06%   41.79%   +0.73%     
   ==========================================
     Files          28       29       +1     
     Lines        1773     1804      +31     
   ==========================================
   + Hits          728      754      +26     
   - Misses        939      942       +3     
   - Partials      106      108       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.14% <60.00%> (+0.24%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [97c5397...487c345](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   @nic-chen need E2E test cases to confirm this bug ^_^


----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (e803038) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/303cc5612feac82dc27305f1c29762262693eda5?el=desc) (303cc56) will **increase** coverage by `0.91%`.
   > The diff coverage is `100.00%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   40.13%   41.05%   +0.91%     
   ==========================================
     Files          26       27       +1     
     Lines        1572     1598      +26     
   ==========================================
   + Hits          631      656      +25     
   - Misses        850      851       +1     
     Partials       91       91              
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.88% <100.00%> (+0.40%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `54.05% <100.00%> (+2.62%)` | :arrow_up: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [303cc56...e803038](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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] tokers commented on a change in pull request #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       > I remember the literal struct initialization is encouraged...
   https://github.com/uber-go/guide/blob/master/style.md#initializing-struct-references
   
   my bad.




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/utils/consts/api_error.go
##########
@@ -27,7 +29,21 @@ func ErrorWrapper(handle WrapperHandle) gin.HandlerFunc {
 	return func(c *gin.Context) {
 		data, err := handle(c)
 		if err != nil {
-			apiError := err.(*ApiError)
+			apiError, ok := err.(*ApiError)
+			if !ok {
+				errMsg := err.Error()
+				if strings.Contains(errMsg, "required") ||

Review comment:
       I had seen the same code in `hadler.go`.
   https://github.com/apache/apisix-dashboard/blob/cb1377cf9859392ee81ee60b3d4e623b1522be1e/api/internal/handler/handler.go#L55-L61
   
   So I think we'd better discuss about the response error.
   
   




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       @tokers we do need an empty entity.SSL object here. what's the recommend way to create an empty object?




----------------------------------------------------------------
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] tokers commented on a change in pull request #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -160,9 +164,9 @@ func (h *Handler) List(c droplet.Context) (interface{}, error) {
 
 	//format respond
 	var list []interface{}
-	var ssl *entity.SSL
 	for _, item := range ret.Rows {
-		ssl = item.(*entity.SSL)
+		ssl := &entity.SSL{}

Review comment:
       Ditto.

##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       Do not use literal struct initialization.
   
   https://github.com/uber-go/guide/blob/master/style.md#toc40

##########
File path: api/internal/utils/consts/api_error.go
##########
@@ -27,7 +29,21 @@ func ErrorWrapper(handle WrapperHandle) gin.HandlerFunc {
 	return func(c *gin.Context) {
 		data, err := handle(c)
 		if err != nil {
-			apiError := err.(*ApiError)
+			apiError, ok := err.(*ApiError)
+			if !ok {
+				errMsg := err.Error()
+				if strings.Contains(errMsg, "required") ||

Review comment:
       Why not encapsulating these logics into a separate method in `ApiError`, like `ApiError.IsInvalidParam`?

##########
File path: api/internal/utils/utils.go
##########
@@ -97,6 +98,16 @@ func InterfaceToString(val interface{}) string {
 	return str
 }
 
+func ObjectClone(origin, copy interface{}) error {

Review comment:
       We'd better to add some comments for this function, such as the object should not have float number, the `json.Marshal` and `json.Unmarshal` will cause the precision loss.




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       @nic-chen ping




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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


   # [Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=h1) Report
   > Merging [#1064](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=desc) (fc1b6ca) into [master](https://codecov.io/gh/apache/apisix-dashboard/commit/cb1377cf9859392ee81ee60b3d4e623b1522be1e?el=desc) (cb1377c) will **increase** coverage by `0.75%`.
   > The diff coverage is `84.61%`.
   
   [![Impacted file tree graph](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/graphs/tree.svg?width=650&height=150&src=pr&token=Q1HERXN96P)](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree)
   
   ```diff
   @@            Coverage Diff             @@
   ##           master    #1064      +/-   ##
   ==========================================
   + Coverage   40.50%   41.25%   +0.75%     
   ==========================================
     Files          27       28       +1     
     Lines        1627     1658      +31     
   ==========================================
   + Hits          659      684      +25     
   - Misses        873      877       +4     
   - Partials       95       97       +2     
   ```
   
   
   | [Impacted Files](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?src=pr&el=tree) | Coverage Δ | |
   |---|---|---|
   | [api/internal/utils/utils.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL3V0aWxzLmdv) | `57.14% <60.00%> (+0.24%)` | :arrow_up: |
   | [api/internal/handler/ssl/ssl.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2hhbmRsZXIvc3NsL3NzbC5nbw==) | `29.54% <66.66%> (+0.06%)` | :arrow_up: |
   | [api/internal/utils/consts/api\_error.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL3V0aWxzL2NvbnN0cy9hcGlfZXJyb3IuZ28=) | `95.65% <100.00%> (ø)` | |
   | [api/internal/core/store/store.go](https://codecov.io/gh/apache/apisix-dashboard/pull/1064/diff?src=pr&el=tree#diff-YXBpL2ludGVybmFsL2NvcmUvc3RvcmUvc3RvcmUuZ28=) | `78.57% <0.00%> (-0.65%)` | :arrow_down: |
   
   ------
   
   [Continue to review full report at Codecov](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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/1064?src=pr&el=footer). Last update [cb1377c...fc1b6ca](https://codecov.io/gh/apache/apisix-dashboard/pull/1064?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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/handler/ssl/ssl.go
##########
@@ -86,7 +86,11 @@ func (h *Handler) Get(c droplet.Context) (interface{}, error) {
 	}
 
 	//format respond
-	ssl := ret.(*entity.SSL)
+	ssl := &entity.SSL{}

Review comment:
       I think it is fine to use `var := &object{}`,and  it is used in many open-source projects.




----------------------------------------------------------------
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 #1064: fix: when enable or disable existing SSL, an error occurred

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



##########
File path: api/internal/utils/utils.go
##########
@@ -97,6 +98,16 @@ func InterfaceToString(val interface{}) string {
 	return str
 }
 
+func ObjectClone(origin, copy interface{}) error {

Review comment:
       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