Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(queue): enhance getJobScheduler method to include template information #2929

Merged
merged 9 commits into from
Dec 2, 2024
Merged

feat(queue): enhance getJobScheduler method to include template information #2929

merged 9 commits into from
Dec 2, 2024

Conversation

roggervalf
Copy link
Collaborator

@roggervalf roggervalf commented Nov 23, 2024 •
edited
Loading

ref #2875

manast
manast reviewed Nov 25, 2024
View reviewed changes
src/classes/job-scheduler.ts Outdated
@@ -257,6 +257,10 @@ export class JobScheduler extends QueueBase {
}
}

async getJobSchedulerTemplate(id: string): Promise<any[]> {
Copy link
Contributor

Choose a reason for hiding this comment

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

As we are in the JobScheduler, I think the proper method name would be: getJobTemplate, also use schedulerId as the first argument to make it more explicit, and add the method's documentation in typedoc format.

src/classes/queue.ts Outdated
const jobScheduler = await this.jobScheduler;
const [jobData, jobId] = await jobScheduler.getJobSchedulerTemplate(id);

return this.createJob(jobData, jobId);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need to return a Job instance here? if it is a template it should not be a Job, just some name, data and options, right?

src/commands/getJobSchedulerTemplate-2.lua Outdated
@@ -0,0 +1,20 @@

--[[
Get job scheduler template
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add a comment here explaining that his is actually taking the last iterations job's data and options, but in the future it will return a stored template in the job scheduler itself, as this is the plan if I am not mistaken.

@roggervalf roggervalf changed the title feat(queue): add getJobSchedulerTemplate method feat(queue): enhance getJobScheduler method to include template information Nov 27, 2024
@roggervalf roggervalf force-pushed the get-job-scheduler-template branch 2 times, most recently from 61d9a15 to 38d39a2 Compare November 30, 2024 19:09
manast
manast reviewed Dec 2, 2024
View reviewed changes
src/classes/job-scheduler.ts
pattern: schedulerAttributes.pattern || null,
every: schedulerAttributes.every || null,
...(schedulerAttributes.data
? {
Copy link
Contributor

Choose a reason for hiding this comment

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

If you are checking if data is defined here, why having a default in the next line?:
data: JSON.parse(schedulerAttributes.data || '{}')
Also, could it potentially be the case that you have an empty data but options?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

yeah I refactored a little bit to not save this data if it's empty, same for template opts

tests/test_job_scheduler.ts
@@ -360,6 +360,12 @@ describe('Job Scheduler', function () {
tz: null,
pattern: '*/2 * * * * *',
every: null,
template: {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe add cases where data / opts are undefined?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

addressed as well

manast
manast approved these changes Dec 2, 2024
View reviewed changes
Copy link
Contributor

@manast manast left a comment

Choose a reason for hiding this comment

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

LGTM

@roggervalf roggervalf merged commit cb99080 into master Dec 2, 2024
12 checks passed
@roggervalf roggervalf deleted the get-job-scheduler-template branch December 2, 2024 20:32
github-actions bot pushed a commit that referenced this pull request Dec 2, 2024
@semantic-release-bot
chore(release): 5.31.0 [skip ci]
9ae31ba 
# [5.31.0](v5.30.1...v5.31.0) (2024-12-02)

### Features

* **queue:** enhance getJobScheduler method to include template information ([#2929](#2929)) ref [#2875](#2875) ([cb99080](cb99080))
alexandresoro pushed a commit to alexandresoro/ouca that referenced this pull request Dec 4, 2024
@alexandresoro
fix(deps): update dependency bullmq to v5.31.1 (#361)
880baea 
This PR contains the following updates:

| Package | Type | Update | Change |
|---|---|---|---|
| [bullmq](https://bullmq.io/) ([source](https://github.com/taskforcesh/bullmq)) | dependencies | minor | [`5.29.1` -> `5.31.1`](https://renovatebot.com/diffs/npm/bullmq/5.29.1/5.31.1) |

---

### Release Notes

<details>
<summary>taskforcesh/bullmq (bullmq)</summary>

### [`v5.31.1`](https://github.com/taskforcesh/bullmq/releases/tag/v5.31.1)

[Compare Source](taskforcesh/bullmq@v5.31.0...v5.31.1)

##### Bug Fixes

-   **scheduler-template:** remove console.log when getting template information ([#&#8203;2950](taskforcesh/bullmq#2950)) ([3402bfe](taskforcesh/bullmq@3402bfe))

### [`v5.31.0`](https://github.com/taskforcesh/bullmq/releases/tag/v5.31.0)

[Compare Source](taskforcesh/bullmq@v5.30.1...v5.31.0)

##### Features

-   **queue:** enhance getJobScheduler method to include template information ([#&#8203;2929](taskforcesh/bullmq#2929)) ref [#&#8203;2875](taskforcesh/bullmq#2875) ([cb99080](taskforcesh/bullmq@cb99080))

### [`v5.30.1`](https://github.com/taskforcesh/bullmq/releases/tag/v5.30.1)

[Compare Source](taskforcesh/bullmq@v5.30.0...v5.30.1)

##### Bug Fixes

-   **flow:** allow using removeOnFail and failParentOnFailure in parents ([#&#8203;2947](taskforcesh/bullmq#2947)) fixes [#&#8203;2229](taskforcesh/bullmq#2229) ([85f6f6f](taskforcesh/bullmq@85f6f6f))

### [`v5.30.0`](https://github.com/taskforcesh/bullmq/releases/tag/v5.30.0)

[Compare Source](taskforcesh/bullmq@v5.29.1...v5.30.0)

##### Bug Fixes

-   **job-scheduler:** upsert template when same pattern options are provided ([#&#8203;2943](taskforcesh/bullmq#2943)) ref [#&#8203;2940](taskforcesh/bullmq#2940) ([b56c3b4](taskforcesh/bullmq@b56c3b4))

##### Features

-   **queue:** add getDelayedCount method \[python] ([#&#8203;2934](taskforcesh/bullmq#2934)) ([71ce75c](taskforcesh/bullmq@71ce75c))
-   **queue:** add getJobSchedulersCount method ([#&#8203;2945](taskforcesh/bullmq#2945)) ([38820dc](taskforcesh/bullmq@38820dc))

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update again.

---

 - [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check this box

---

This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).
<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzOC4xNDIuNyIsInVwZGF0ZWRJblZlciI6IjM4LjE0Mi43IiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6WyJkZXBlbmRlbmNpZXMiXX0=-->

Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/361
Reviewed-by: Alexandre Soro <code@soro.dev>
Co-authored-by: renovate <renovate@git.tristess.app>
Co-committed-by: renovate <renovate@git.tristess.app>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@manast manast manast approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@roggervalf @manast