-
Notifications
You must be signed in to change notification settings - Fork 7
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
Cert health check can use duration syntax for deadline param #188
Conversation
- Ex: /api/health/certs?deadline=7d **Why**: This allows us to query the endpoint with fixed strings for separate durations, so we could warn at a 30d expiration but have a seprate check that errors at a 7d expiration
**Why**: To match IDP configs
@@ -1,31 +0,0 @@ | |||
--- |
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.
We had already disabled reek in #165, but this file stuck around. While tracking down the method complexity error CodeClimate error I found this and figured I'd remove it. Can do another PR if anybody feels strongly
def parse | ||
return if value.blank? | ||
|
||
match = value.match(/^(?<number>\d+)(?<duration>\D)$/) |
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.
I guess the switch fallthrough would have the same effect of generating a nil
value, but... should we match on the specific duration characters we support?
match = value.match(/^(?<number>\d+)(?<duration>\D)$/) | |
match = value.match(/^(?<number>\d+)(?<duration>[dwmy])$/) |
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.
My thinking was the switch fallthrough would be the easiest? If we ever add a new character we only need to add it on one place, also allows like multicharacter signifiers if we ever wanted to allow like hr
and wk
or something
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.
My thinking was the switch fallthrough would be the easiest? If we ever add a new character we only need to add it on one place, also allows like multicharacter signifiers if we ever wanted to allow like
hr
andwk
or something
Seems fine to me. And I just now noticed you have test coverage for this with the 123x
example, so should be protected against accidental regressions. On that point, I guess my primary concern is not that the switch fallthrough doesn't work, just that it might not be as obvious to the next person that we're relying on it.
when 'w' # weeks | ||
(7 * number).days | ||
when 'm' # months | ||
(30 * number).days | ||
when 'y' # years | ||
(365 * number).days |
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.
Are we okay that these aren't strictly accurate? e.g. months with fewer or more than 30 days, or leap years.
Would it work to just...
when 'w' # weeks | |
(7 * number).days | |
when 'm' # months | |
(30 * number).days | |
when 'y' # years | |
(365 * number).days | |
when 'w' # weeks | |
number.weeks | |
when 'm' # months | |
number.months | |
when 'y' # years | |
number.years |
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.
When originally implementing this I decided to make it simpler/dumber so I could easily document it:
See verified_within
(inside the details dropdown) on https://developers.login.gov/oidc/
The ActiveSupport methods are very nice but they use fractional quantities for some of those so it seemed more predictable this way (so when the fractional quantities add up they don't change hours/minutes as much)
1.years.to_i.to_f / 1.day.to_i
=> 365.2425
I feel somewhat strongly about leaving it simpler and documented, but if you feel strongly, I could change it here and discuss changing the IDP with the rest of the team
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.
I feel somewhat strongly about leaving it simpler and documented, but if you feel strongly, I could change it here and discuss changing the IDP with the rest of the team
I don't feel strongly.
👍
Why: This allows us to query the endpoint with fixed strings
for separate durations, so we could warn at a 30d expiration but
have a separate check that errors at a 7d expiration
Notes: I cribbed the
DurationParser
class I wrote in the IDP. It didn't seem worth forcing into a shared gem or anything, for reference, it's at: https://github.com/18F/identity-idp/blob/master/app/services/duration_parser.rb