Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: expose JSON Schema through API #1592

Merged
merged 13 commits into from
Mar 17, 2021
Merged

Conversation

nic-chen
Copy link
Member

Please answer these questions before submitting a pull request


New feature or improvement

  • Describe the details and related test reports.

The APISIX ingress controller needs to use the json schema (dashboard FE or other projects may also need it), and it still use the admin api interface.
At present, the manager api only supports to expose the json schema of plugins.

so we now support exposing JSON Schema of all resources and plugins through API


Please add the corresponding test cases if necessary.

added.

@juzhiyuan juzhiyuan requested review from starsz, gxthrj and LiteSun March 14, 2021 16:04
@juzhiyuan
Copy link
Member

also cc @guoqqqi to review

@codecov-io
Copy link

codecov-io commented Mar 14, 2021

Codecov Report

Merging #1592 (8edf1a9) into master (01c9528) will increase coverage by 1.12%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1592      +/-   ##
==========================================
+ Coverage   71.46%   72.58%   +1.12%     
==========================================
  Files         132       86      -46     
  Lines        5375     2356    -3019     
  Branches      592      592              
==========================================
- Hits         3841     1710    -2131     
+ Misses       1290      646     -644     
+ Partials      244        0     -244     
Flag Coverage Δ
backend-e2e-test ?
backend-e2e-test-ginkgo ?
backend-unit-test ?
frontend-e2e-test 72.58% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
api/internal/filter/cors.go
api/internal/utils/utils.go
api/internal/handler/tool/tool.go
api/internal/handler/healthz/healthz.go
api/internal/core/store/validate.go
api/internal/handler/handler.go
api/internal/handler/data_loader/route_import.go
api/internal/handler/data_loader/route_export.go
api/internal/core/storage/etcd.go
api/internal/conf/conf.go
... and 34 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 01c9528...8edf1a9. Read the comment docs.

Copy link
Member

@bisakhmondal bisakhmondal left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work 👍
Just minor changes

"github.com/onsi/ginkgo"
"github.com/onsi/ginkgo/extensions/table"

"e2enew/base"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2enew module path has been updated in #1578

Suggested change
"e2enew/base"
"github.com/apisix/manager-api/test/e2enew/base"

api/test/e2enew/schema/schema_test.go Outdated Show resolved Hide resolved
@moonming
Copy link
Member

why this PR will decrease coverage by 12.25%?

@nic-chen
Copy link
Member Author

why this PR will decrease coverage by 12.25%?

Because the coverage of the last PR merged into the master only counts the coverage of frontend-e2e-test, see:

#1587 (comment)

And in this PR it’s calculated
The coverage of frontend-e2e-test and backend-e2e-test, so the statistical result is decreased.

We have contacted codecov to see if we can solve this unstable problem.

if res, ok := conf.Plugins[pluginName]; !ok || !res {
continue
if pluginName != "serverless-post-function" && pluginName != "serverless-pre-function" {
ret = append(ret, pluginName)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The logic had changed?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

api/internal/handler/schema/plugin_test.go Show resolved Hide resolved
api/internal/handler/schema/schema.go Show resolved Hide resolved
@@ -23,5 +23,5 @@ import (
)

func TestPlugin(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rename to TestSchema?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed.

Copy link
Contributor

@gxthrj gxthrj left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@starsz
Copy link
Contributor

starsz commented Mar 16, 2021

@LiteSun Help looks at the Frontend CI.

@LiteSun
Copy link
Member

LiteSun commented Mar 16, 2021

@LiteSun Help looks at the Frontend CI.

OK, I will check it.

func (h *SchemaHandler) Schema(c droplet.Context) (interface{}, error) {
input := c.Input().(*SchemaInput)

ret := conf.Schema.Get("main." + input.Resource).Value()
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wht not return an error like "schema not found" when the schema doesn't exist.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

updated. thanks.

table.DescribeTable("test plugin basic", func(testCase base.HttpTestCase) {
base.RunTestCase(testCase)
},
table.Entry("get all plugins", base.HttpTestCase{
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Keep the indent aligned with table.DescribeTable.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh, this is formatted by go fmt @tokers

@LiteSun
Copy link
Member

LiteSun commented Mar 17, 2021

Copy link
Member

@Jaycean Jaycean left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@nic-chen nic-chen requested a review from tokers March 17, 2021 02:45
Copy link
Contributor

@bzp2010 bzp2010 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

😆👍

@LiteSun LiteSun merged commit 4b8774f into apache:master Mar 17, 2021
@nic-chen nic-chen deleted the expose-schema branch March 17, 2021 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

feat: support exposing schema of route, upstream, service and ssl through Manager API