-
Notifications
You must be signed in to change notification settings - Fork 0
Unify timestamp checks #57
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
Conversation
|
This approach could also be incorporated into the generic "Review Process" check set that we were considering making (i.e. checks that are applicable to all the different phases) |
switch to target subjects for timestamps so parent id captured
douglowe
left a comment
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 think this is an elegant solution. There's a couple places we have redundant code now, which could be removed.
rocrate_validator/profiles/five-safes-crate/must/12_check_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/must/12_check_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/must/13_validation_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/must/13_validation_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/must/8_disclosure_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/must/8_disclosure_phase.ttl
Outdated
Show resolved
Hide resolved
rocrate_validator/profiles/five-safes-crate/should/4_sign_off.ttl
Outdated
Show resolved
Hide resolved
douglowe
left a comment
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.
This all looks good to me now.
Alternative to #56 focused only on unifying the approach to timestamp validation. Adds a combined check into the new file
3_timestamp_format.ttlas I noticed we haven't used the index 3 for any other checks. Example error message:Downside: the error messages don't state which entity the problem timestamp is from. But I think overall it's cleaner than the option presented in #56/#50