Closed bhearsum closed 7 months ago
I note that a 14 digit invalid date (hour == 25 or something like that) will still be problematic. Maybe better to let it fail and handle the failure? But I don't have anything specific in mind, and this seems pragmatic.
That's a good call out - I'm going to see if I can come up with something here. (Failures in the cronjob are almost invisible...so it's good to try and protect against this...)
I note that a 14 digit invalid date (hour == 25 or something like that) will still be problematic. Maybe better to let it fail and handle the failure? But I don't have anything specific in mind, and this seems pragmatic.
That's a good call out - I'm going to see if I can come up with something here. (Failures in the cronjob are almost invisible...so it's good to try and protect against this...)
@jcristau suggested we try to get errors sent to Sentry - which I think is a great idea. I opened #3087 for this, and I'm not going to pursue any other improvements to this PR at this time.
I discovered this issue while testing the cleanup script in stage, which had a release with a name like "Firefox-mozilla-central-nightly-20170731100325-2". The dry run of the cleanup script worked fine, but when it tried to delete it threw an error about not being able to parse it as a date. After some digging, I discovered that this is because the dryrun's
SELECT
uses an index, while theDELETE
does not. (I don't know why this is - but it doesn't affect the behaviour of the script beyond making theDELETE
look at some rows that it won't be deleting anayways...)In any case, we may as well make sure the names we're about to parse are actually parsable.