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

Do not rate limit in dev #1574

Closed
wants to merge 3 commits into from
Closed

Do not rate limit in dev #1574

wants to merge 3 commits into from

Conversation

paulmelnikow
Copy link
Member

I ran into a rate limit while running the service tests, which seems not ideal!

I wonder if it would be better to set RATE_LIMIT=true on the server and default to false.

@paulmelnikow paulmelnikow added the developer-experience Dev tooling, test framework, and CI label Mar 15, 2018
@shields-ci
Copy link

shields-ci commented Mar 15, 2018

Warnings
⚠️

This PR modified package.json, but not package-lock.json - Perhaps you need to run npm install?

Messages
📖

✨ Thanks for your contribution to Shields, @paulmelnikow!

Generated by 🚫 dangerJS

@paulmelnikow
Copy link
Member Author

@RedSparr0w Would you mind giving this a look?

@RedSparr0w
Copy link
Member

RedSparr0w commented Mar 19, 2018

Yeah this sounds good, doesn't make sense to rate limit ourselves,
Would whitelisting localhost/127.0.0.1 also work?

const ipRateLimit = new RateLimit({
whitelist: /^192\.30\.252\.\d+$/, // Whitelist GitHub IPs.
});
const badgeTypeRateLimit = new RateLimit({maxHitsPerPeriod: 3000});
const refererRateLimit = new RateLimit({
maxHitsPerPeriod: 300,
whitelist: /^https?:\/\/shields\.io\/$/,
});

@paulmelnikow
Copy link
Member Author

Would whitelisting localhost/127.0.0.1 also work?

That is a great idea! Maybe that would be a simpler way to make this change.

@paulmelnikow
Copy link
Member Author

Hmm, this code doesn't have any tests, though. Maybe it's better to merge the fix I'm confident in.

# Conflicts:
#	package.json
@RedSparr0w
Copy link
Member

I can't seem to hit the rate limit, I've tried setting:

    this.period = 5000; 
    this.maxHitsPerPeriod = 10;

and also set RATE_LIMIT=true,
But still seems to pass the limit.

Even made 2 test using the static badge which ran 3500 times each and went through fine.
/badge/test-test-blue.json
/badge/test${i}-test${i}-blue.json

@paulmelnikow
Copy link
Member Author

Hmm, did you run the service tests? The static badges aren't rate limited. Not sure whether cached badges would be, either.

@RedSparr0w
Copy link
Member

That would probably be why then, just tried it on the static badge.
Will give it another go on new badge using a local JSON resource.

@paulmelnikow
Copy link
Member Author

@RedSparr0w I've updated this. Would you mind taking it for another spin?

Copy link
Member

@RedSparr0w RedSparr0w left a comment

Choose a reason for hiding this comment

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

I still can't manage to get the server to rate limit no matter what i try, but i can't see this doing any harm.

@paulmelnikow
Copy link
Member Author

Since you haven't been able to reproduce this, I think I'll close it until I run into the problem again.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
developer-experience Dev tooling, test framework, and CI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants