Skip to content

feat: Introduce new overwrite flag to jobs#8701

Merged
dapr-bot merged 17 commits into
dapr:masterfrom
javier-aliaga:javi-jobs-create-if-not-exists
May 23, 2025
Merged

feat: Introduce new overwrite flag to jobs#8701
dapr-bot merged 17 commits into
dapr:masterfrom
javier-aliaga:javi-jobs-create-if-not-exists

Conversation

@javier-aliaga

@javier-aliaga javier-aliaga commented May 7, 2025

Copy link
Copy Markdown
Contributor

Description

This PR contains a new flag to specify if a job can overwrite an existing one with the same name.

The default value is false. This changes the current behaviour as of now the job is overwriten by default

Actor Reminders can be scheduled using the scheduler or state store as storage. In that case we will always overwrite jobs to be consistent with state store implementation

PRs to follow:

Issue reference

Please reference the issue this PR will close: #8402

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

nelson-parente
nelson-parente previously approved these changes May 7, 2025

@nelson-parente nelson-parente left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

LGTM :)

@javier-aliaga javier-aliaga changed the title [WIP] feat: Introcude new overwrite flag to jobs [WIP] feat: Introduce new overwrite flag to jobs May 7, 2025
@javier-aliaga javier-aliaga changed the title [WIP] feat: Introduce new overwrite flag to jobs feat: Introduce new overwrite flag to jobs May 7, 2025
@javier-aliaga
javier-aliaga marked this pull request as ready for review May 7, 2025 13:41
@javier-aliaga
javier-aliaga requested review from a team as code owners May 7, 2025 13:41
The new flags can be use to specify if the job can overwrite an existing one.
The default value is `false`

Signed-off-by: Javier Aliaga <[email protected]>
@javier-aliaga
javier-aliaga force-pushed the javi-jobs-create-if-not-exists branch from ddab7ea to 1533137 Compare May 9, 2025 11:15
@javier-aliaga javier-aliaga changed the title feat: Introduce new overwrite flag to jobs [WIP] feat: Introduce new overwrite flag to jobs May 9, 2025
@javier-aliaga
javier-aliaga marked this pull request as draft May 9, 2025 11:34
Comment thread pkg/actors/internal/reminders/storage/scheduler/scheduler.go
@javier-aliaga javier-aliaga changed the title [WIP] feat: Introduce new overwrite flag to jobs feat: Introduce new overwrite flag to jobs May 12, 2025
@javier-aliaga
javier-aliaga marked this pull request as ready for review May 12, 2025 17:34
Comment thread tests/integration/suite/daprd/jobs/grpc/basic.go Outdated
Comment thread tests/integration/suite/daprd/jobs/grpc/basic.go Outdated
Comment thread tests/integration/suite/daprd/jobs/grpc/basic.go Outdated
Comment thread tests/integration/suite/daprd/jobs/http/basic.go Outdated
Move overwrite tests to its own files

Signed-off-by: Javier Aliaga <[email protected]>
Comment on lines +80 to +81
{"overwrite2", `{"schedule": "@daily", "repeats": 3, "due_time": "10s", "ttl": "11s"}`, http.StatusInternalServerError},
{"overwrite2", `{"schedule": "@daily", "repeats": 3, "overwrite": false, "due_time": "10s", "ttl": "11s"}`, http.StatusInternalServerError},

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I would expect a HTTP status Conflict here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Probably this is somthing we want to do for all errors here ? I have checked all other errors and they are not respecting neither the grpc code or the http code

Comment thread tests/integration/suite/daprd/jobs/http/overwrite.go
Comment thread tests/integration/suite/daprd/jobs/grpc/overwrite.go
Signed-off-by: Javier Aliaga <[email protected]>
Convert grc error code to http error code on http requests for schedule jobs

Signed-off-by: Javier Aliaga <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
@javier-aliaga
javier-aliaga requested a review from JoshVanL May 20, 2025 10:19
@JoshVanL JoshVanL added autoupdate DaprBot will keep the Pull Request up to date with master branch automerge Allows DaprBot to automerge and update PR if all approvals are in place labels May 20, 2025
Comment thread pkg/scheduler/server/api.go Outdated
Comment thread pkg/scheduler/server/api.go Outdated
javier-aliaga and others added 2 commits May 20, 2025 15:23
Co-authored-by: Josh van Leeuwen <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
Co-authored-by: Josh van Leeuwen <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
@javier-aliaga
javier-aliaga requested a review from JoshVanL May 20, 2025 13:24
JoshVanL
JoshVanL previously approved these changes May 20, 2025
Comment thread pkg/api/errors/scheduler.go Outdated
@cicoyle cicoyle removed the automerge Allows DaprBot to automerge and update PR if all approvals are in place label May 21, 2025
Comment thread pkg/api/errors/scheduler.go Outdated
Comment thread tests/integration/suite/daprd/jobs/grpc/overwrite.go
Comment thread tests/integration/suite/daprd/jobs/grpc/overwrite.go Outdated
Comment thread tests/integration/suite/daprd/jobs/http/overwrite.go Outdated
Comment thread tests/integration/suite/daprd/jobs/http/overwrite.go
Better assertions
Import ordering

Signed-off-by: Javier Aliaga <[email protected]>
Signed-off-by: Javier Aliaga <[email protected]>
@javier-aliaga
javier-aliaga requested a review from cicoyle May 21, 2025 18:32
Comment thread tests/integration/suite/daprd/jobs/http/overwrite.go
Comment thread tests/integration/suite/daprd/jobs/http/overwrite.go
@cicoyle cicoyle added the automerge Allows DaprBot to automerge and update PR if all approvals are in place label May 21, 2025
@dapr-bot
dapr-bot merged commit 137da85 into dapr:master May 23, 2025
@javier-aliaga
javier-aliaga deleted the javi-jobs-create-if-not-exists branch August 5, 2025 07:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

automerge Allows DaprBot to automerge and update PR if all approvals are in place autoupdate DaprBot will keep the Pull Request up to date with master branch

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Don't overwrite jobs using same name

5 participants