Skip to content

fix: add admin schema to control_plane config#8809

Merged
spacewander merged 2 commits into
apache:masterfrom
jiangfucheng:fix_8804
Feb 15, 2023
Merged

fix: add admin schema to control_plane config#8809
spacewander merged 2 commits into
apache:masterfrom
jiangfucheng:fix_8804

Conversation

@jiangfucheng

Copy link
Copy Markdown
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)

@spacewander spacewander left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@jiangfucheng

Copy link
Copy Markdown
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.

@spacewander spacewander left a comment

Copy link
Copy Markdown
Member

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
Copy Markdown
Member Author

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

@shreemaan-abhishek

Copy link
Copy Markdown
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.

Comment thread t/cli/test_deployment_control_plane.sh Outdated
' > conf/config.yaml

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

Copy link
Copy Markdown
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
Copy Markdown
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。🤣

Comment thread apisix/cli/schema.lua Outdated
},
},
required = {"etcd", "role_control_plane"}
required = {"etcd", "admin", "role_control_plane"}

Copy link
Copy Markdown
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
Copy Markdown
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
Comment thread t/cli/test_deployment_control_plane.sh Outdated
cert: t/certs/mtls_server.crt
cert_key: t/certs/mtls_server.key
admin:
https_admin: "abc"

Copy link
Copy Markdown
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
Copy Markdown
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