-
Notifications
You must be signed in to change notification settings - Fork 84
Conversation
Actually, this doesn't fix the enabled issue, see Justin's comment on the issue. Also, add a couple tests |
@@ -38,6 +38,11 @@ private void ParseRules(XElement rules, IList<IISUrlRewriteRule> result) | |||
return; | |||
} | |||
|
|||
if(string.Equals(rules.Name.ToString(), "GlobalRules", StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space
It does fix the enabled flag issue. Justin was mistaken |
else | ||
{ | ||
builder.Enabled = enabled; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if !Enabled, consider not parsing the rest of the rule. That would allow users to disable rules that use features we don't support, rather than having to delete them.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's a good idea. The only thing is that if we don't parse the rest of the rule we allow the user to enter invalid rules since the checking isn't happening. Not a huge deal since the rule won't be enabled bit still something to consider
Ping @Tratcher @BrennanConroy |
For issues #138 and #139.
I also made the middleware throw when global rules are present since we don't actually handle them properly with the current implementation.
@Tratcher @muratg @BrennanConroy