feat: refactor apis for existing check and other apis by nic-chen · Pull Request #535 · apache/apisix-dashboard

nic-chen · 2020-10-04T14:15:19Z

Please answer these questions before submitting a pull request

Why submit this pull request?
Bug fix
New feature provided
Improve performance

New feature or improvement

Describe the details and related test reports.

feat: refactor apis for existing check and other apis
such as:
/apisix/admin/notexist/upstreams
/apisix/admin/names/upstreams
/apisix/admin/notexist/routes
/apisix/admin/check_ssl_cert

and so on.

gxthrj · 2020-10-05T09:37:14Z

+ * 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.
+ */


Duplicate header

ping @nic-chen

gxthrj · 2020-10-05T09:39:21Z

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// Copyright 2017 The Kubernetes Authors.


are these headers must to be kept ?

Is this file from Kubernetes repo?

some code copy from Kubernetes dashboard , so i think it's better to keep their license. what do you think ? @moonming

ping @moonming

moonming · 2020-10-05T10:51:06Z

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// Copyright 2017 The Kubernetes Authors.


Is this file from Kubernetes repo?

moonming · 2020-10-05T10:51:20Z

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// Copyright 2017 The Kubernetes Authors.


moonming · 2020-10-05T10:51:32Z

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// Copyright 2017 The Kubernetes Authors.


moonming · 2020-10-05T13:35:01Z

This method is not right. What is the license of the original code? nic-chen <[email protected]>于2020年10月5日周一下午9:31写道：

***@***.**** commented on this pull request. ------------------------------ In api/internal/core/store/query.go <#535 (comment)> : > + * 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. + */ +// Copyright 2017 The Kubernetes Authors. some code copy from Kubernetes dashboard , so i think it's better to keep their license. what do you think ? @moonming <https://github.com/moonming> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#535 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AGJZBKYXWZRPE5RE5STNBV3SJHDBRANCNFSM4SDXIAEQ> . --

Thanks, Ming Wen Twitter: _WenMing

nic-chen · 2020-10-05T13:37:14Z

This method is not right. What is the license of the original code?

the Apache License, Version 2.0

moonming · 2020-10-06T00:53:23Z

Please add codecov

nic-chen · 2020-10-09T02:33:09Z

ping @moonming @ShiningRush @gxthrj @juzhiyuan

moonming · 2020-10-09T06:36:14Z

we can leave the lincense issues when merge to master. @nic-chen

moonming · 2020-10-09T06:36:49Z

@nic-chen what's the code coverage?

membphis · 2020-10-09T06:11:20Z

+ * 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.
+ */


ping @nic-chen

membphis · 2020-10-09T06:11:55Z

+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+// Copyright 2017 The Kubernetes Authors.


ping @moonming

membphis · 2020-10-09T06:14:54Z

 	r.PATCH("/apisix/admin/ssl/:id", wgin.Wraps(h.Patch,
 		wrapper.InputType(reflect.TypeOf(UpdateInput{}))))
-	r.DELETE("/apisix/admin/ssl", wgin.Wraps(h.BatchDelete,
+	r.DELETE("/apisix/admin/ssl/:ids", wgin.Wraps(h.BatchDelete,


why we use ids here? I think it should be id, the singular should be used here.

nic-chen · 2020-10-09T07:57:07Z

@nic-chen what's the code coverage?

you could see in my repo:
https://github.com/nic-chen/incubator-apisix-dashboard/pull/7/checks?check_run_id=1212879646

main code of refactoring:
coverage: 81.3% of statements
github.com/apisix/manager-api/internal/core/store
coverage: 63.3% of statements
github.com/apisix/manager-api/internal/utils

the handlers using api test, so it couldn't calculate coverage correctly.

nic-chen · 2020-10-09T07:58:05Z

@membphis we leave the license issues and fix it later.

nic-chen · 2020-10-09T07:59:32Z

I will merge this pr.
so that we could move forward.
we could fix other issues later.

@moonming @membphis @juzhiyuan @ShiningRush

moonming · 2020-10-09T08:01:39Z

I will merge this pr.
so that we could move forward.
we could fix other issues later.

@moonming @membphis @juzhiyuan @ShiningRush

please create issues for them

nic-chen · 2020-10-09T08:08:38Z

I will merge this pr.
so that we could move forward.
we could fix other issues later.
@moonming @membphis @juzhiyuan @ShiningRush

please create issues for them

ok

juzhiyuan · 2020-10-09T08:12:11Z

Please link this PR in those issues :D

nic-chen · 2020-10-09T08:23:27Z

Please link this PR in those issues :D

OK

* feat: refactor some codes and append store core * refference droplet and write a API demo * fmt project * feat: add validator for generic store; add demo error * feat: upgrade droplet * chore: add structures (#484) * add upstream struct * fix structures * feat: append stock check for generic store * feat: add consumer CURD refactoring (#486) * feat: add consumer CURD refactoring * remove debug * remove useless slashes * feat: add SSL refactoring (#488) Co-authored-by: Vinci Xu <[email protected]> * feat: add Service and Upstream refactoring (#497) * feat: add Service and Upstream refactoring * fix add license * feat: add store hub and flake id (#534) * feat: add store hub and flake id * feat: add store interface to easy est * fix: add test cases for refactored apis and fix bugs (#528) * test: delete mysql version test cases. * fix: add ssl test cases and bug fix * fix: list api should return an empty array not a null for client * test: consumer test cases * fix: code style * test: init etcd in github action * fix: skip checking generated file's license * feat: add store hub and flake id (#534) * feat: add store hub and flake id * feat: add store interface to easy est * fix CI failed Co-authored-by: Vinci Xu <[email protected]> * feat: refactor apis for existing check and other apis (#535) * fix code style * fix code style * feat support query * feat: support query * change: `like` to `equal` * fix api status * feat: upstream existing check * feat: refactor api for upstream names * fix: license * test: add unit test cases * fix: update bug * test: add test cases for route * fix: unified respond format * test: remove test bug * feat: ssl existing check * fix bug: auto generate id * fix: improve consumer * fix: remove key and keys in ssl respond * fix: when list is empty, should respond an empty array * fix code style * feat: plugin orchestration * fix delete bug * fix bug * fix: keep the same request params and respond with the old format * fix: append sort for list and using sync.Map instead of map * feature: sync json schema from APISIX and check schema when create or update resource (#551) * feat: json schema check * fix: don't need to define struct for each resource, because that may cause json schema check fail. * test: add handler test cases * test: complete consumer test cases * test: add test cases for schema check * fix code style and license * feat: add schema check for plugins * test: add ssl handler test cases * test: add test cases for upstream and service * test: add test cases for route * test: add note for route create * test: update CI * fix: remove useless file * test: fix CI * fix: ci fail * test: fix lib `dag-to-lua`'s path in CI * fix: URI for route may be empty * fix: remove empty lines * fix: refactor validator of json schema * fix code style * fix cicd * chore: update docker file * fix: should check schema after id generated * fix code style * chore: page_number -> page * fix: schema sync script * fix: code style * feat: support search for resource list (#557) * feat: support search for resource list * fix ci * feature: refactor plugin api and auth api (#556) * feat: refactor plugin and healthy api * feat: refactor authentication api * fix: remove useless files * chore: update json schema * test: add login test * test: add test for plugin * fix: license * fix auth bug * fix route search by uri * feat: compatible with PUT method of `admin api` and nodes of upstream (#561) * feat: support labels * feat: compatible with PUT method of `admin api` * fix mock test fail * feat: upstream nodes format * test: add test case * fix code style * fix: update schema sync tool * feat: compatible with HTTP status of `admin api` (#563) * feat: compatible with HTTP status of `admin api` * test cases and improve * fix: check input.ID before using it * fix: remove CI branch * fix: remove useless dependences * add license for json.lua * fix license issue * remove * fix: remove viper that depend github.com/hashicorp/hcl * fix license issue * fix: skip license check temporarily for CI Co-authored-by: vincixu <[email protected]> Co-authored-by: ShiningRush <[email protected]> Co-authored-by: WenMing <[email protected]>

johzchen added 14 commits October 2, 2020 18:54

fix code style

5dc137b

fix code style

1903243

feat support query

b8a5f8d

feat: support query

241d5f9

change: like to equal

038f673

fix api status

bc40bb2

feat: upstream existing check

358f53e

feat: refactor api for upstream names

c122536

fix: license

da70dc3

test: add unit test cases

17cbcc3

fix: update bug

9fbf8fa

test: add test cases for route

ef1babc

fix: unified respond format

e1f8f49

test: remove test bug

2747a0f

nic-chen marked this pull request as ready for review October 5, 2020 09:05

nic-chen requested review from ShiningRush, gxthrj, juzhiyuan, membphis and moonming and removed request for juzhiyuan October 5, 2020 09:06

gxthrj reviewed Oct 5, 2020

View reviewed changes

moonming requested changes Oct 5, 2020

View reviewed changes

feat: ssl existing check

cda5d48

johzchen added 2 commits October 7, 2020 18:29

fix bug: auto generate id

4ded654

fix: improve consumer

26af99f

johzchen added 7 commits October 7, 2020 20:39

fix: remove key and keys in ssl respond

b3b3c4a

fix: when list is empty, should respond an empty array

4106036

fix code style

848ffbf

feat: plugin orchestration

d0f1bd6

fix delete bug

cb1180e

fix bug

0a55cf4

fix: keep the same request params and respond with the old format

9b61a51

membphis reviewed Oct 9, 2020

View reviewed changes

nic-chen merged commit caccfb7 into apache:refactor Oct 9, 2020

nic-chen mentioned this pull request Oct 9, 2020

[refactor] license issue of refactored api #546

Closed

5 tasks

Uh oh!

Conversation

nic-chen commented Oct 4, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New feature or improvement

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

moonming commented Oct 5, 2020 via email

Uh oh!

nic-chen commented Oct 5, 2020

Uh oh!

moonming commented Oct 6, 2020

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

moonming commented Oct 9, 2020

Uh oh!

moonming commented Oct 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

moonming commented Oct 9, 2020

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

juzhiyuan commented Oct 9, 2020

Uh oh!

nic-chen commented Oct 9, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

nic-chen commented Oct 4, 2020 •

edited

Loading