schedule.time's data type interpretation may be confusing

[aisamu]

Some YAML serializers transform a string of the form "hh:mm" into a value without quotes.

❯ node
Welcome to Node.js v12.22.1.
> const yaml = require('yaml')

> console.log(yaml.stringify({time: "12:00"}))
time: 12:00

> console.log(yaml.stringify({time: "1200"}))
time: "1200"

Current behavior in GitHub-native Dependabot:

Using a YAML file containing an unquoted schedule.time field fails with:

The property '#/updates/0/schedule/time' of type integer did not match the following type: string

Basic info:

Package ecosystem: npm

Please note that I'm not sure if that's a bug on Dependabot or on the serializer.
Regardless of the root cause, it may be useful to handle those cases to aid those less acquainted with YAML's corner cases (like myself)!
Alternatively, I'd happily use a dependabot.json.

Thanks for writing this tool!
It's tremendously valuable for us.

[aisamu]

IIUC, that's apparently because Dependabot is not using YAML 1.2, which "fixed" the ambiguity by dropping some unorthodox datatypes: eemeli/yaml#264

[jurre]

Hi @aisemu, the time field currently takes a string, not a Time object, I think we can clarify that in the docs.

The error you're seeing is from the json-schema validation that we run on the parsed yaml object.

Longer-term, we're hoping to provide a cron-based syntax for scheduling, although there is no planned timeline for that yet.

[aisamu]

Hi @jurre!

Thanks for your reply!

the time field currently takes a string, [...], I think we can clarify that in the docs.

That is known, and the error message makes that clear!
That's unfortunately not the issue, though.
In your reply, you mention:

, not a Time object,

YAML is not parsing 12:00 as a Time object, but as base-60 integer.
From the last entry on the v1.1 int spec:

Resolution and Validation:

    Valid values must match the following regular expression, which may also be used for implicit tag resolution:

     [-+]?0b[0-1_]+ # (base 2)
    |[-+]?0[0-7_]+ # (base 8)
    |[-+]?(0|[1-9][0-9_]*) # (base 10)
    |[-+]?0x[0-9a-fA-F_]+ # (base 16)
    |[-+]?[1-9][0-9_]*(:[0-5]?[0-9])+ # (base 60)

Example 1. !!int Examples

canonical: 685230
decimal: +685_230
octal: 02472256
hexadecimal: 0x_0A_74_AE
binary: 0b1010_0111_0100_1010_1110
sexagesimal: 190:20:30

This was apparently deemed as a confusing feature, so the YAML 1.2 spec dropped it.

Longer-term, we're hoping to provide a cron-based syntax for scheduling, although there is no planned timeline for that yet.

Although that would be very nice, perhaps some simpler adjustments would suffice!
More specifically, this would be solved by specifying the YAML spec to be >= 1.2 while parsing dependabot.yml.
An alternative syntax that doesn't collide with sexagesimal numbers (such as 12h30) would also do the trick.

[asciimike]

Hey @aisamu, long time no see ;)

My quick take is: I don't think we care about supporting sexigesimal numbers, so I'd be fine supporting yaml 1.2 (the other changes it makes seem fine to me). We use ruby's default yaml parser (which is Psych, which is a wrapper around libyaml). Psych doesn't have the option to pick a yaml version, nor does it look like libyaml does.

The fix here is going to be once libyaml supports 1.2 properly (see yaml/libyaml#20) you'll have the option to use a YAML directive to specify which version you're using, and you can select 1.2 to get this behavior (it may become the default, I'm not sure). Note, I tried directives in a dependabot.yml a few minutes ago and it failed, so it's not clear to me if Psych has pulled in the changes from yaml/libyaml#172 or, maybe more realistically, the version of Ruby we're on probably hasn't pulled in the changes from the latest version of Psych (if only there was an easy way for them to keep their dependencies updated...). We could probably use Psych directly once they have 1.2 support fully baked, but that's probably more of a refactor than we'd like to do as this doesn't seem like an issue affecting that many folks.

I'm going to close this for now as it will be fixed when our dependencies support it, but feel free to re-open if there's any new info!