NightlyBase doesn't convert string in dayOfWeek param #1424
Conversation
No conversion is made of the value in dayOfWeek if it's a string. The
value is directly passed to croniter without proper conversion.
This fix handles both single string values and multiple comma separated
values as well as abbreviation in the string. ("0" "5,6" "0,fri" et.c.)
| if isDayOfWeek: | ||
| time_array = str(time).split(',') | ||
| # string could be a comma separated list of values, e.g. "5,sun" | ||
| for i, t in enumerate(time_array): |
There was a problem hiding this comment.
[nit] t is a too short non-descriptive name
There was a problem hiding this comment.
I hoped you wouldn't notice :)
I'll fix that immediately.
…ring_in_dayofweek NightlyBase doesn't convert string in dayOfWeek param
|
Thank you :) |
|
@gangefors thanks for this. However, the docs aren't specific at all about what is or is not a valid value for Nightly's time-related parameters. In the absence of this patch, I'd say Buildbot's docs should just link to croniter's docs and leave it at that. However, with this patch things are a little more complicated, as we're supporting a superset of croniter's allowed inputs. Do you think you could make another pull req to fix up the documentation to be as unambiguous as possible? |
|
I found this issue when I was trying to get a scheduler running on only weekends. I didn't pass these values as lists of integers since I got those values from my code as comma separated strings. How do you suggest I update the docs? Should I just specify that you are able to pass strings and even comma separated vaule strings with this fix? Add to the examples maybe? And why would the '5,6' string support be a superset of croniter allowed inputs? It's a valid crontab value and therefore allowed as input to croniter according to their docs which only links to Cron wikipedia page. |
|
I think the main issue is that the current documentation says "each may be a single number or a list of valid values", which does not cover the new possibilities :) So, at least, this needs to be extended. On the other hand, a link to the croniter docs would be nice. That'd be it from my point of view, I do not know though what @djmitche had in mind. |
|
My comment about a superset was regarding pre-processing the inputs to croniter. If (as in I think Buildbot's docs should point more clearly to the croniter docs. If the croniter implementation is lacking (in not taking comma-separated strings, for instance) or its docs are lacking (by just pointing to a wiki page) then that should be fixed in croniter, not patched in by Buildbot. |
|
dayOfWeek is always processed before passing to croniter since buildbot uses mon=0 instead of sun=0. If buildbot had used mon=1 and sun=7 croniter would accept that wihtout processing any input from buildbot, no matter if it was a integer or a string. Since buildbot have a different offset for dayOfWeek it needs processing, but this is done only if it's an integer (single or list of). Nightly accepts strings without complaining, but with a erroneous result (triggers one day early) when passing a single string value to dayOfWeek. If the documentation had been explicit about only allowing integers (in list or single) then it should throw a exception when passing strings. But since strings works except for dayOfWeek I wanted to fix this. You can give a string like "1" or "1,2,3" or even "jan,feb,mar" as month and it will trigger the same as 1 or [1,2,3]. But that won't work with dayOfWeek before this patch was applied. I don't see a single integer value as a string ("1") as a superset of croniter allowed input, or "1,2" for that matter. This format is just not specified in the buildbot docs but is accepted and works for all except dayOfWeek where the values aren't processed as they are when using integers. This patch fixes that, but it doesn't add any extra features that croniter doesn't already accept. I'd be happy to update the documentation to reflect that strings are valid to use and point to croniter docs regarding what value formats are allowed. But I'm not sure if that's what you want me to do? |
|
Ah, I hadn't looked at this code in a while and was going off what I saw in a look at the diff. You're absolutely right -- we're already doing a good bit of pre-processing ( So yes, the fix is good, the tests are good, and I think we just need to be a little clearer in the documentation that names and comma-separated strings are allowed. The only string explicitly mentioned in the docs is |
No conversion is made of the value in dayOfWeek if it's a string. The
value is directly passed to croniter without proper conversion.
This fix handles both single string values and multiple comma separated
values as well as abbreviation in the string. ("0" "5,6" "0,fri" et.c.)