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

fix: add admin schema to control_plane config #8809

Merged
merged 2 commits into from
Feb 15, 2023

Conversation

jiangfucheng
Copy link
Member

Description

Fixes #8804

Maybe just add the schema of admin to conrol_plane config definition is enough?

Checklist

  • I have explained the need for this PR and the problem it solves
  • I have explained the changes or the new features added to this PR
  • I have added tests corresponding to this change
  • I have updated the documentation to reflect this change
  • I have verified that this change is backward compatible (If not, please discuss on the APISIX mailing list first)

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

Can you add a test to validate the admin schema in https://github.com/apache/apisix/blob/master/t/cli/test_deployment_control_plane.sh?
So we won't miss schema next time.

@jiangfucheng
Copy link
Member Author

Can you add a test to validate the admin schema in https://github.com/apache/apisix/blob/master/t/cli/test_deployment_control_plane.sh? So we won't miss schema next time.

I added test case for this pr. I wanted to through schema.validate() method to validate whether config is right, but admin field already in default-config.yaml, schema.validate() can't return any error, so i deleted admin field in config-default.yaml.

Copy link
Member

@spacewander spacewander left a comment

Choose a reason for hiding this comment

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

You can provide an incorrect type in

role = {type = "string"},

The basic rule: try your best to avoid touching the functional code when you write test

@jiangfucheng jiangfucheng force-pushed the fix_8804 branch 2 times, most recently from 4b90878 to b1eb207 Compare February 9, 2023 14:49
@jiangfucheng
Copy link
Member Author

@spacewander I can't reproduce the failed CI, could you guide me how to fix it?

@shreemaan-abhishek
Copy link
Contributor

@jiangfucheng You can make an empty commit and push it to re-run the CI. This might be some error that does not occur consistently.

' > conf/config.yaml

out=$(make init 2>&1 || true)
if ! echo "$out" | grep 'expect: number, but got: string'; then
Copy link
Member

Choose a reason for hiding this comment

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

The err is from there:

return nil, "failed to merge, path[" .. path .. "] expect: " ..

It is detected before the schema check.
We should find another way to validate the schema check.

Copy link
Member Author

Choose a reason for hiding this comment

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

I changed a special field, it can be merged, but can't pass config validation, although this way is not perfect。🤣

@@ -409,7 +410,7 @@ local deployment_schema = {
default = {},
},
},
required = {"etcd", "role_control_plane"}
required = {"etcd", "admin", "role_control_plane"}
Copy link
Member

Choose a reason for hiding this comment

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

The admin is not required as we always have a default value.

Copy link
Member Author

Choose a reason for hiding this comment

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

Removed.

spacewander
spacewander previously approved these changes Feb 14, 2023
cert: t/certs/mtls_server.crt
cert_key: t/certs/mtls_server.key
admin:
https_admin: "abc"
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
https_admin: "abc"
https_admin: "abc"

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

@spacewander spacewander merged commit 00e9a56 into apache:master Feb 15, 2023
@jiangfucheng jiangfucheng deleted the fix_8804 branch February 16, 2023 06:16
hongbinhsu added a commit to fitphp/apix that referenced this pull request Feb 23, 2023
* upstream/master:
  fix: limit count plugin conf parameter undefined error (apache#8902)
  chore: remove duplicate kubernetes test case (apache#8882)
  feat: add 'range_id' algorithm for 'request-id' plugin (apache#8790)
  chore(error-log-logger): add kafka meta_refresh_interval (apache#8821)
  chore(deps): bump golang.org/x/net from 0.2.0 to 0.7.0 in /t/grpc_server_example (apache#8881)
  docs: Update getting-started.md (apache#8763)
  fix(admin): fix wrong http code for patch method (apache#8855)
  feat: stream subsystem support tars service discovery (apache#8826)
  feat(ci): implement image caching to reduce ci build time. (apache#8735)
  feat(admin): add head method support to /apisix/admin (apache#8752)
  feat: opentelemetry plugin config collector.address support specify https scheme (apache#8823)
  fix: add admin schema to control_plane config (apache#8809)
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.

chore: Supplement the schema in deployment
4 participants