This repository has been archived by the owner on Jan 1, 2020. It is now read-only.
[api/silenced] allow for subscriptions containing colons *and* hyphens #1457
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| @@ -3,7 +3,7 @@ module API | |||
| module Routes | |||
| module Silenced | |||
| SILENCED_URI = /^\/silenced$/ | |||
| SILENCED_SUBSCRIPTION_URI = /^\/silenced\/subscriptions\/([\w\.-:]+)$/ | |||
| SILENCED_SUBSCRIPTION_URI = /^\/silenced\/subscriptions\/([\w\.:-]+)$/ | |||
There was a problem hiding this comment.
It's traditional to put a - first in a character class. Or \escape it.
There was a problem hiding this comment.
I'll escape it for clarity. 👍
…criptions Some of the test assertions already committed weren't running because they were not nested within a single block and async_done was being called too early. This change ensures those assertions run, removes some assertions that aren't particularly relevant to this test, and creates test examples for a variety of subscription names using colons, hyphens and underscores to exercise the regexp used by the /silenced/subscriptions/:subscription API endpoint.
…RI regexps I think we should escape the hyphen here for purposes of indicating that we want to match a hyphen and not to indicate a range.
portertech
reviewed
Sep 21, 2016
| "load-balancer", | ||
| "load_balancer", | ||
| "loadbalancer" | ||
| ].each do |sub| |
There was a problem hiding this comment.
The only criticism that I have for you, why not subscription? We've avoided the use of short and single character variables when possible.
There was a problem hiding this comment.
Other then changing this variable, 👍
|
@portertech ready for merge |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
In #1450 we introduced a change to the regexp for validating requests to the
/silenced/subscriptions/:subscriptionendpoint, but this change introduced a regression preventing the handling of subscriptions that also contain a hyphen (e.g. HTTP GET against/silenced/subscriptions/client:fooworks but HTTP GET agains/silenced/subscriptions/client:foo-bardoes not).Transposing the positions of-and:in the regex seems to solve the problem.In the interests of clarity and readability I've opted to escape the hyphen rather than change its position.
Related Issue
Closes #1449
Motivation and Context
The regexp for matching valid subscriptions under /silenced/subscriptions route
excludes valid subscriptions containing colons and hyphens, e.g.
client:foo-bar.How Has This Been Tested?
Expanded the unit test coverage to test a variety of subscriptions using colons, hyphens and underscores in various combinations.
Types of changes
Checklist: