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

Issue #26: Support handling configuration files without read permissions #102

Merged
merged 17 commits into from
Nov 25, 2019

Conversation

mdimjasevic
Copy link
Contributor

This PR addresses the following remark from issue #26: "When lyft/ratelimit can't read a file (e.g. chmod 0-ed), other files are loaded correctly and it responds to requests with OK." Fencer now matches the behavior.

Also see a discussion in an issue #26 comment.

@mdimjasevic mdimjasevic self-assigned this Nov 13, 2019
( tests
, writeAndLoadRules
-- example values
, domain1Text
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's not have different *.Test files depending on each other.

Let's also not have something like domain1Text in an export list, the name does not describe the semantics.

You can have a module called RuleExamples or something and reexport rules from there.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I created a separate module Fencer.Rules.Test.Examples with these example values and renamed them to better suite what they represent.

@mdimjasevic
Copy link
Contributor Author

@neongreen, I'll proceed with merging this PR. In case there are still things you would like to see changed, please let me know in any of the PRs depending on this one.

@mdimjasevic mdimjasevic merged commit bf41f51 into master Nov 25, 2019
@mdimjasevic mdimjasevic deleted the mdimjasevic/26-cant-read-file branch November 25, 2019 14:24
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.

3 participants