Skip to content

dev: fix schema not accepting valid timeout #5501

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

Merged
merged 3 commits into from
Mar 3, 2025

Conversation

SuperSandro2000
Copy link
Contributor

@SuperSandro2000 SuperSandro2000 commented Mar 3, 2025

Tested with https://regex101.com/r/7h45UC/1

While doing sapcc/go-makefile-maker#248 we noticed that valid time.Durations are not accepted in the schema which got validated by the github action with verify=true (https://github.com/sapcc/go-makefile-maker/actions/runs/13635720861/job/38113890543?pr=248) but the command itself ran fine locally.

After this change everything works fine when running the following locally:

./golangci-lint config verify -c ~/go-makefile-maker/.golangci.yaml --schema jsonschema/golangci.jsonschema.json

Verified

This commit was signed with the committer’s verified signature.
Josh-Cena Joshua Chen
SuperSandro2000 added a commit to sapcc/go-makefile-maker that referenced this pull request Mar 3, 2025

Verified

This commit was signed with the committer’s verified signature.
Josh-Cena Joshua Chen
SuperSandro2000 added a commit to sapcc/go-makefile-maker that referenced this pull request Mar 3, 2025
SuperSandro2000 added a commit to sapcc/go-makefile-maker that referenced this pull request Mar 3, 2025
@ldez ldez self-requested a review March 3, 2025 17:12
@ldez ldez changed the title Fix schema not accepting valid time.Duration format dev: fix schema not accepting valid timeout Mar 3, 2025
Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this regexp works: ^(\d+[sm]?){1,2}$

@ldez
Copy link
Member

ldez commented Mar 3, 2025

The suggested regexp doesn't work because it matches 3m0m, m, s and doesn't match 0.

My previous suggestion doesn't work too because it matches 3m0m or 3s0m.

I think the right expression is ^(\d+m\d+s|\d+m|\d+s|\d)$

@ldez ldez added the bug Something isn't working label Mar 3, 2025
@ldez ldez added this to the unreleased milestone Mar 3, 2025
@ldez
Copy link
Member

ldez commented Mar 3, 2025

Inside time.ParseDuration we can found this regexp [-+]?([0-9]*(\.[0-9]*)?[a-z]+)+.
https://github.com/golang/go/blob/0312e31ed197b3bf1434e8dbb283f0d2374d7457/src/time/format.go#L1622

But this regexp is not a validation regexp because it accepts everything. https://regex101.com/r/odfWT9/1

In the context of run.timeout:

  • The values that use "ns", "us" (or "µs"), "ms" are off-topic.
  • The value "0" is accepted.
  • The negative values are off-topic.

So the best compromise is ^((\d+h)?(\d+m)?(\d+(?:\.\d)?s)?|0)$.

https://regex101.com/r/1Zse4r/1

@ldez
Copy link
Member

ldez commented Mar 3, 2025

The duration parsing is a bit surprising: https://go.dev/play/p/-NSON-ylfvm
ex: 3m3m3m -> 9m0s

So, at the end the regexp can be ^((\d+[hms])+|0)$.

I don't know if I want to follow the "official" duration format (without negative numbers and too small units) or keep my latest suggestion with something more "logical". 🤔

@ldez
Copy link
Member

ldez commented Mar 3, 2025

🤔 I think it's better to have something more strict and less human error prone.
Using 3m3m3m as value to have 9m for the timeout is "unexpected", I cannot see a use-case for that.
Same thing for 30s5m, except if you are a Yoda fan, I cannot see a use-case for that too.

Copy link
Member

@ldez ldez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ldez ldez merged commit e1eb4cb into golangci:master Mar 3, 2025
18 checks passed
@SuperSandro2000 SuperSandro2000 deleted the timeout-schema branch March 3, 2025 20:13
@SuperSandro2000
Copy link
Contributor Author

Thanks! that was quick

SuperSandro2000 added a commit to sapcc/go-makefile-maker that referenced this pull request Mar 3, 2025
…lved"

This reverts commit bdb3316.
@ldez ldez modified the milestones: unreleased, v1.64 Mar 12, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area: JSON schema bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants