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.

Sign up for GitHub

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

Jump to bottom

feat: extend chaitin-waf plugin functionalities #9945 #12029

Merged
merged 10 commits into from
Mar 28, 2025
Merged

feat: extend chaitin-waf plugin functionalities #9945 #12029

merged 10 commits into from
Mar 28, 2025

Conversation

AlyHKafoury
Copy link
Contributor

@AlyHKafoury AlyHKafoury commented Mar 9, 2025 •
edited
Loading

Description

Fixed #9945

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)

AlyHKafoury
AlyHKafoury commented Mar 10, 2025
View reviewed changes
@AlyHKafoury AlyHKafoury marked this pull request as ready for review March 10, 2025 16:48
@dosubot dosubot bot added size:L This PR changes 100-499 lines, ignoring generated files. enhancement New feature or request plugin labels Mar 10, 2025
@AlyHKafoury AlyHKafoury marked this pull request as draft March 10, 2025 16:49
@AlyHKafoury AlyHKafoury marked this pull request as ready for review March 10, 2025 17:15
@dosubot dosubot bot added size:XL This PR changes 500-999 lines, ignoring generated files. and removed size:L This PR changes 100-499 lines, ignoring generated files. labels Mar 10, 2025
@AlyHKafoury
Copy link
Contributor Author

@membphis I believe this is ready for review

@AlyHKafoury
Copy link
Contributor Author

@moonming A gentle ping that this PR is ready for review

@moonming
Copy link
Member

@moonming A gentle ping that this PR is ready for review

Cool, I will review your PR, thanks for your contribution

@AlyHKafoury
Copy link
Contributor Author

I will Fix linting issues

Baoyuantop
Baoyuantop reviewed Mar 12, 2025
View reviewed changes
Copy link
Contributor

@Baoyuantop Baoyuantop left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution! There are some suggestions for improvement on the documents.

@AlyHKafoury
Copy link
Contributor Author

@Baoyuantop Thank you for your comments, I applied them

@Baoyuantop
Copy link
Contributor

@Baoyuantop Thank you for your comments, I applied them

Can you help fix the failed CI? If it's unstable, let us know and we'll re-run it.

@AlyHKafoury
Copy link
Contributor Author

AlyHKafoury commented Mar 12, 2025 via email

@AlyHKafoury
Copy link
Contributor Author

@Baoyuantop A gentle reminder

Baoyuantop
Baoyuantop previously approved these changes Mar 13, 2025
View reviewed changes
Copy link
Contributor

@Baoyuantop Baoyuantop left a comment

Choose a reason for hiding this comment

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

LGTM, waiting for others to review

@Baoyuantop Baoyuantop requested review from membphis and moonming March 13, 2025 02:34
moonming
moonming previously approved these changes Mar 19, 2025
View reviewed changes
AlinsRan
AlinsRan reviewed Mar 19, 2025
View reviewed changes
@AlyHKafoury AlyHKafoury dismissed stale reviews from moonming and Baoyuantop via 471526a March 21, 2025 11:51
@AlyHKafoury
Copy link
Contributor Author

@AlinsRan @Baoyuantop @moonming @membphis I completed the latest requested changes, and the tests pass locally so please review and help run the CI tests

@AlyHKafoury
Copy link
Contributor Author

This CI fail run is unstable, please restart the failed job

@Baoyuantop
Copy link
Contributor

cc @membphis help review

nic-6443
nic-6443 reviewed Mar 27, 2025
View reviewed changes
apisix/plugins/chaitin-waf.lua
Comment on lines +57 to +58
enum = { "off", "monitor", "block", nil },
default = nil,
Copy link
Member

Choose a reason for hiding this comment

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

We don't need to explicitly specify nil in enum and default value. When this field is not declared as required in the JSON schema, it can be nil, and the default value is also nil.

Copy link
Contributor

Choose a reason for hiding this comment

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

This does not affect the use of this feature, which is merged first and then an issue is created to track it.

nic-6443
nic-6443 reviewed Mar 27, 2025
View reviewed changes
apisix/plugins/chaitin-waf.lua
Comment on lines +103 to +104
enum = { "off", "monitor", "block", nil },
default = nil,
Copy link
Member

Choose a reason for hiding this comment

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

ditto

@Baoyuantop Baoyuantop merged commit 3825674 into apache:master Mar 28, 2025
33 checks passed
@Baoyuantop Baoyuantop mentioned this pull request Mar 28, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@nic-6443 nic-6443 nic-6443 left review comments

@moonming moonming moonming approved these changes

@Baoyuantop Baoyuantop Baoyuantop approved these changes

@AlinsRan AlinsRan AlinsRan approved these changes

@membphis membphis Awaiting requested review from membphis

Assignees
No one assigned
Labels
enhancement New feature or request plugin size:XL This PR changes 500-999 lines, ignoring generated files.
Projects
Apache APISIX backlog
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

feat: extend chaitin-waf plugin functionalities
5 participants
@AlyHKafoury @moonming @Baoyuantop @nic-6443 @AlinsRan