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 missing properties to schema #10339

Merged
merged 23 commits into from
Nov 20, 2023

Conversation

pottekkat
Copy link
Member

@pottekkat pottekkat commented Oct 16, 2023

Description

Adds missing properties to the schema so that it can be used for validating configuration files more accurately.

Also consolidates all the schemas to a single schema instead of having to validate with multiple schemas.

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)

Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
@pottekkat pottekkat marked this pull request as draft October 16, 2023 07:36
@pottekkat pottekkat marked this pull request as ready for review October 17, 2023 11:31
@pottekkat
Copy link
Member Author

I'm unsure why the tests are failing.

@Revolyssup
Copy link
Contributor

Revolyssup commented Oct 17, 2023

@pottekkat Can you change this to string? The test will pass
https://github.com/apache/apisix/pull/10339/files#diff-33a40d0c58eba6fec065b7df0f75fd0637387555e3ad0170d8e6674948575093R573

Because you can see this is a string and not number.

client_max_body_size: 512m

apisix/cli/schema.lua Outdated Show resolved Hide resolved
Co-authored-by: Ashish Tiwari <[email protected]>
@pottekkat
Copy link
Member Author

pottekkat commented Oct 17, 2023

@Revolyssup I got this error before so I tried changing it:

https://github.com/apache/apisix/actions/runs/6549717930/job/17787244082?pr=10339#step:5:287

But it should be string though.

@pottekkat
Copy link
Member Author

@Revolyssup Should we add a case where it can be either a string or a number?

Signed-off-by: Navendu Pottekkat <[email protected]>
@monkeyDluffy6017
Copy link
Contributor

LGTM, That's a pretty big change. So let's make the ci pass

@monkeyDluffy6017 monkeyDluffy6017 added the wait for update wait for the author's response in this issue/PR label Oct 20, 2023
dependencies = {
role = {
oneOf = {
{
Copy link
Contributor

Choose a reason for hiding this comment

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

@pottekkat Will you make sure that this dependency schema fits right with this[1] yaml snippet here?

  1. https://github.com/apache/apisix/blob/88406dceab26ed7f25b663088b0b5e760bd45a38/t/cli/test_validate_config.sh#L210C1-L210C1

Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
@pottekkat
Copy link
Member Author

I reverted to the same changes as in the last commit where the tests were passing in this commit: bf8bdb8

But now, the tests fail.

I'm now trying by removing all changes and checking if the tests pass now and build from there.

Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
@pottekkat
Copy link
Member Author

All checks pass with these changes.

Signed-off-by: Navendu Pottekkat <[email protected]>
@pottekkat
Copy link
Member Author

It seems like the issue might have been in the etcd schema and with merging the config and deployment schemas. Checking what the issue is incrementally.

Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
Signed-off-by: Navendu Pottekkat <[email protected]>
@monkeyDluffy6017 monkeyDluffy6017 added approved and removed wait for update wait for the author's response in this issue/PR user responded labels Nov 20, 2023
@monkeyDluffy6017
Copy link
Contributor

@Revolyssup please help to check

@monkeyDluffy6017 monkeyDluffy6017 merged commit c8f99fb into apache:master Nov 20, 2023
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants