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

Add support for multiple limit_req statements in location directives #1570

Merged
merged 2 commits into from
Nov 13, 2023

Conversation

ltning
Copy link
Contributor

@ltning ltning commented Jul 18, 2023

Pull Request (PR) description

Adds support for multiple limit_req statements in a location by allowing limit_zone to be passed as an array OR a string.

This Pull Request (PR) fixes the following issues

Fixes #1569

@ltning
Copy link
Contributor Author

ltning commented Oct 9, 2023

Can someone look at this PR, please? I have no idea why the checks are not successful; I'm a sporadic contributor with correspondingly sparse knowledge of how to pursue this - but the improvement is both trivial and useful (to us, posisbly others).

Copy link
Member

@smortex smortex left a comment

Choose a reason for hiding this comment

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

CI fails because of (hard to see in the output):

REFERENCE.md is outdated

Please update REFERENCE.md with

$ bundle exec rake strings:generate:reference

Add it to the PR and CI should continue.

templates/server/location_header.erb Outdated Show resolved Hide resolved
@ltning
Copy link
Contributor Author

ltning commented Oct 10, 2023

CI fails because of (hard to see in the output):

REFERENCE.md is outdated

Please update REFERENCE.md with

$ bundle exec rake strings:generate:reference

Add it to the PR and CI should continue.

Doing this changes nothing..? I have never used bundle or any of these tools, so I feel extremely noob right now..

@ltning
Copy link
Contributor Author

ltning commented Oct 31, 2023

Dum-did-um @smortex any ideas? :)

@smortex
Copy link
Member

smortex commented Oct 31, 2023

REFERENCE.md is outdated

Please update REFERENCE.md with:

$ bundle exec rake strings:generate:reference

Add it to the PR and CI should continue.

@kenyon kenyon force-pushed the multi-limit-zone branch 2 times, most recently from 7afecf5 to 6d95b4e Compare November 1, 2023 05:03
@kenyon
Copy link
Member

kenyon commented Nov 1, 2023

I updated REFERENCE.md and squashed the commits.

@kenyon
Copy link
Member

kenyon commented Nov 1, 2023

The unit test needs some work still.

@ltning
Copy link
Contributor Author

ltning commented Nov 1, 2023

What kind of work? Anything I can/should do?

@kenyon
Copy link
Member

kenyon commented Nov 1, 2023

@ltning yes, see the test failure. Something about the matching isn't right.

Co-authored-by: Romain Tartière <romain@blogreen.org>
@ltning
Copy link
Contributor Author

ltning commented Nov 10, 2023

Sorry @smortex - I don't understand what's still failing here. I see it passing on most, but not all, test runs.

@kenyon
Copy link
Member

kenyon commented Nov 10, 2023

Ah, it's the same error we're seeing in #1579 (@C24-AK), but seeing it on Puppet 7 there versus 8 here. So it doesn't have to do with the changes in these pull requests.

@kenyon kenyon changed the title Add support for multiple limit_req statements in locations Add support for multiple limit_req statements in location directives Nov 13, 2023
@kenyon kenyon added the enhancement New feature or request label Nov 13, 2023
@kenyon kenyon merged commit 7f11626 into voxpupuli:master Nov 13, 2023
25 of 27 checks passed
@TheMeier TheMeier added this to the 6.0.0 milestone Jun 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

limit_zone should take Array in addition to String and Undef
4 participants