Skip to content
This repository has been archived by the owner on Feb 24, 2024. It is now read-only.

Extend worker.Worker to support periodic job registration #2308

Closed
wants to merge 1 commit into from
Closed

Extend worker.Worker to support periodic job registration #2308

wants to merge 1 commit into from

Conversation

acaloiaro
Copy link
Contributor

@acaloiaro acaloiaro commented Sep 5, 2022 •
edited
Loading

Extend worker.Worker to support periodic job registration, in support of gobuffalo/gocraft-work-adapter#6

The purpose of this PR is to invite commentary on adding periodic job support. A concrete worker.Simple implementation would follow, along with tests.

@acaloiaro acaloiaro requested a review from a team as a code owner September 5, 2022 16:20
@sio4 sio4 self-assigned this Sep 5, 2022
@sio4 sio4 added documentation Improvements or additions to documentation s: triage Some tests need to be run to confirm the issue enhancement New feature or request and removed documentation Improvements or additions to documentation labels Sep 5, 2022
@sio4
Copy link
Member

sio4 commented Sep 5, 2022

Please change the merge target to v1. The main branch is not yet ready.

@acaloiaro acaloiaro changed the base branch from main to v1 September 5, 2022 17:28
@sio4
Copy link
Member

sio4 commented Sep 7, 2022

Hi @acaloiaro, sorry for my late response.

It seems like you worked based on the main branch, which is not yet prepared. (I am working on setting the next version's tree on there, but not yet finished) Please rebase your fix on the v1 branch. Also, it could be better if you run the basic test on your local env before pushing the change.

@sio4 sio4 removed the s: triage Some tests need to be run to confirm the issue label Sep 26, 2022
@github-actions
Copy link

This PR is stale because it has been open 45 days with no activity. Remove stale label or comment. Otherwise, this will be closed in 7 days.

@github-actions github-actions bot added the stale label Nov 11, 2022
@acaloiaro
Extend worker.Worker to support periodic job registration
5961a47 
- Provide a concrete periodic `worker.Simple` implementation
@acaloiaro
Copy link
Contributor Author

@sio4

I went ahead and rebased, and provided a concrete Simple implementation using github.com/robfig/cron as the cron implementation. This is the same dependency underlying https://github.com/gocraft/work and consequentially https://github.com/gobuffalo/gocraft-work-adapter

@acaloiaro acaloiaro mentioned this pull request Nov 13, 2022
Closed
@github-actions github-actions bot removed the stale label Nov 14, 2022
sio4
sio4 reviewed Nov 19, 2022
View reviewed changes
worker/worker.go
@@ -24,6 +24,8 @@ type Worker interface {
PerformIn(Job, time.Duration) error
// Register a Handler
Register(string, Handler) error
// RegisterPeriodic performs a job periodically according to the provided cron spec
RegisterPeriodic(cronSpec, jobName string, h Handler) error
Copy link
Member

Choose a reason for hiding this comment

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

First, this is a breaking change invalidates all existing (official/unofficial/custom) implementations. It should be considered as a separate interface or extended when we redesign the Worker support in a future major version.
(will add more comments in #2351)

@sio4 sio4 added breaking change This feature / fix introduces breaking changes proposal A suggestion for a change, feature, enhancement, etc labels Nov 19, 2022
@acaloiaro
Copy link
Contributor Author

Closing for now while a more holistic design is considered in #2242

@acaloiaro acaloiaro closed this Nov 29, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@sio4 sio4 sio4 left review comments

Assignees

@sio4 sio4

Labels
breaking change This feature / fix introduces breaking changes enhancement New feature or request proposal A suggestion for a change, feature, enhancement, etc
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@acaloiaro @sio4