Bug 1714432 - Review Request: golang-github-robfig-cron - Cron library for go
Summary: Review Request: golang-github-robfig-cron - Cron library for go
Keywords:
Status: NEW
Alias: None
Product: Fedora
Classification: Fedora
Component: Package Review
Version: rawhide
Hardware: All
OS: Linux
medium
medium
Target Milestone: ---
Assignee: Nobody's working on this, feel free to take it
QA Contact: Fedora Extras Quality Assurance
URL:
Whiteboard:
Depends On:
Blocks:
TreeView+ depends on / blocked
 
Reported: 2019-05-28 06:31 UTC by Mark Goodwin
Modified: 2019-07-15 15:01 UTC (History)
5 users (show)

Fixed In Version:
Doc Type: If docs needed, set a value
Doc Text:
Clone Of:
Environment:
Last Closed:
Type: ---


Attachments (Terms of Use)

Comment 1 Elliott Sales de Andrade 2019-05-28 08:29:32 UTC
There are some tags; is there any reason not to be packaging one of those?

Comment 2 Mark Goodwin 2019-05-28 22:34:42 UTC
Hi Elliot,

the v3.0.0-rc1 tag is not released (rc1). As per the README.md:

      It is currently IN DEVELOPMENT and will be considered released once a 3.0 version is tagged. It is backwards INCOMPATIBLE with both the v1 and v2 branches.

So I decided to package the HEAD commit version (and will update to the v3.0.0 version once it is tagged).

This is for grafana-6.2.x and later, which needs v3 (v1 and v2 are incompatible).

Regards and thanks

Comment 3 Elliott Sales de Andrade 2019-05-29 07:38:04 UTC
Yes, but the v3.0.0-rc1 tag is the same as the HEAD of the v3 branch, so it'd be the same code, just with a proper version.

Comment 4 Mark Goodwin 2019-05-29 08:33:33 UTC
(In reply to Elliott Sales de Andrade from comment #3)
> Yes, but the v3.0.0-rc1 tag is the same as the HEAD of the v3 branch, so
> it'd be the same code, just with a proper version.

grafana has vendored HEAD of the master branch, which is different (newer) to HEAD of the v3 branch.
That leads me to think final tagged v3.0.0 (when it happens) will be same (or close) to current master, and hence why grafana went with that rather than current v3.

Regards

Comment 5 Elliott Sales de Andrade 2019-05-29 21:42:50 UTC
I tried to run fedora-review, but the tests fail:

2019/05/29 17:25:40 cron: panic running job: YOLO
goroutine 23 [running]:
github.com/robfig/cron.(*Cron).runWithRecovery.func1(0xc0000b8410)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:161 +0xa0
panic(0x5574fb915300, 0x5574fb949740)
        /usr/lib/golang/src/runtime/panic.go:522 +0x1b9
github.com/robfig/cron.TestFuncPanicRecovery.func1()
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron_test.go:19 +0x3b
github.com/robfig/cron.FuncJob.Run(0x5574fb948ed8)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:92 +0x27
github.com/robfig/cron.(*Cron).runWithRecovery(0xc0000b8410, 0x5574fb94c0e0, 0x5574fb948ed8)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:165 +0x59
created by github.com/robfig/cron.(*Cron).run
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:199 +0x74b
2019/05/29 17:25:41 cron: panic running job: YOLO
goroutine 24 [running]:
github.com/robfig/cron.(*Cron).runWithRecovery.func1(0xc0000f4000)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:161 +0xa0
panic(0x5574fb915300, 0x5574fb949770)
        /usr/lib/golang/src/runtime/panic.go:522 +0x1b9
github.com/robfig/cron.DummyJob.Run(...)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron_test.go:30
github.com/robfig/cron.(*Cron).runWithRecovery(0xc0000f4000, 0x5574fb94c0c0, 0x5574fba4a5c0)
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:165 +0x59
created by github.com/robfig/cron.(*Cron).run
        /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/src/github.com/robfig/cron/cron.go:199 +0x74b
--- FAIL: TestNonLocalTimezone (2.02s)
    cron_test.go:261: expected job fires 2 times
FAIL
exit status 1
FAIL    github.com/robfig/cron  25.439s

Comment 6 Mark Goodwin 2019-05-31 01:03:22 UTC
(In reply to Elliott Sales de Andrade from comment #5)
> I tried to run fedora-review, but the tests fail:
> 
[...]
> /builddir/build/BUILD/cron-b41be1df696709bb6395fe435af20370037c0b4c/_build/
> src/github.com/robfig/cron/cron.go:199 +0x74b
> --- FAIL: TestNonLocalTimezone (2.02s)
>     cron_test.go:261: expected job fires 2 times
> FAIL
> exit status 1
> FAIL    github.com/robfig/cron  25.439s

Hi Elliott, thanks once again.

I ran fedora-review three times - the first failed (as you saw too), but then 2nd and 3rd runs passed.
It seems the TestNonLocalTimezone / cron_test.go may be racy (in terms of how many times the job fires).

I'll open an upstream issue about that. In the mean-time, I'll put this review request on hold - may be best to do anyway until v3.0.0 is tagged. Grafana can continue to vendor github.com/robfig/cron until then.

Regards
-- Mark

Comment 7 Elliott Sales de Andrade 2019-05-31 07:08:38 UTC
If I've read the test correctly, it looks like it schedules a job for 1 second and 2 seconds in the future (in absolute time), waits 2 seconds and checks it's been called twice. But given a bit of jitter, this is probably a bit fragile.

Either 2 seconds is not long enough to wait (because the initial setup took a little while so the second fire is not exactly 2 seconds later, but a smidge longer), or the first job didn't fire (because Now() was calculated just a bit before a rollover to the next second.) Or possibly both of these.

Comment 8 Robert-André Mauchin 2019-07-10 22:34:13 UTC
Packaged as golang-gopkg-robfig-cron-3 already. Maybe you could update it to package the tip of the v3 branch?
Are you sure Grafana can't work with the v3 version? If it builds, it should be ok.

Comment 9 Mark Goodwin 2019-07-11 22:51:22 UTC
I posted issue #205 https://github.com/robfig/cron/issues/205 (based on Elliott's Comment #7) which has now been fixed along with a bunch of other issues; and he has now released and tagged v3.0.0. So I'll update golang-gopkg-robfig-cron-3 to that release rather than the tip of v3 branch (probably the same anyway), and recheck grafana.

Comment 10 Mark Goodwin 2019-07-15 01:43:51 UTC
zebob, the v3.0.0 tagged release uses golang modules, which has resulted in issues like https://github.com/robfig/cron/issues/209  

So the import path would have to be github.com/robfig/cron/v3 (and gopkg.in/robfig/cron.v3 will no longer work). I'm not sure what this kind of thing means for the macros work just merged into rawhide :P but for now I think we should keep golang-gopkg-robfig-cron-3 as it currently is.

Comment 11 Robert-André Mauchin 2019-07-15 15:01:46 UTC
github.com/robfig/cron/v3 won't work as we don't have modules enabled in Fedora yet.


Note You need to log in before you can comment on or make changes to this bug.