-
Notifications
You must be signed in to change notification settings - Fork 41
helper to list workflow registration #157
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
helper to list workflow registration #157
Conversation
maxdml
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the PR, @PhakornKiong. This is going on a good direction!
I think we can just reuse the existing workflowRegistryEntry, make it public and add a CronSchedule property, instead of adding two new types.
This also means we can keep using a single map to store all the workflows. The list functions can filter in/out the scheduled workflows.
dbos/workflows_test.go
Outdated
| if dbosCtx.(*dbosContext).workflowScheduler != nil { | ||
| ctx := dbosCtx.(*dbosContext).workflowScheduler.Stop() | ||
| <-ctx.Done() // Wait for it to stop | ||
| dbosCtx.(*dbosContext).workflowScheduler = nil | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We shouldn't have to do this: setupDBOS calls Shutdown on the context, which stops the scheduler.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed. I was getting the following error. Realized that I did not launch dbos. Just sharing here for future reference
time=2025-10-01T03:38:37.431+08:00 level=INFO msg="Initializing DBOS context" app_name=test-app dbos_version=(devel)
time=2025-10-01T03:38:37.432+08:00 level=INFO msg="Connecting to system database" database_url="postgres://postgres:***@localhost:5432/dbos?sslmode=disable" schema=dbos
time=2025-10-01T03:38:38.111+08:00 level=INFO msg="Registered scheduled workflow" fqn=github.com/dbos-inc/dbos-transact-golang/dbos.simpleWorkflowWithSchedule cron_schedule="0 0 * * * *"
time=2025-10-01T03:38:38.111+08:00 level=INFO msg="Cleaning up DBOS instance..."
--- FAIL: TestRegisteredWorkflowListing (1.28s)
/dbos/dbos-transact-golang/dbos/utils_test.go:76: found unexpected goroutines:
[Goroutine 50 in state select, with github.com/robfig/cron/v3.(*Cron).run on top of the stack:
github.com/robfig/cron/v3.(*Cron).run(0xc00032a320)
/go/pkg/mod/github.com/robfig/cron/[email protected]/cron.go:263 +0xdf0
created by github.com/robfig/cron/v3.(*Cron).Start in goroutine 19
/go/pkg/mod/github.com/robfig/cron/[email protected]/cron.go:222 +0x100
]
FAIL
FAIL github.com/dbos-inc/dbos-transact-golang/dbos 1.613s
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch. This must be because we do start the scheduler in registerScheduledWorkflow. We should do it in Launch only. I'll fix it!
I've updated the PR with the following changes:
|
maxdml
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM! Thanks @PhakornKiong. Let's just make WrappedFunction private -- no need to expose it to the public API.
dbos/queue.go
Outdated
| } | ||
|
|
||
| _, err := registeredWorkflow.wrappedFunction(ctx, input, WithWorkflowID(workflow.id)) | ||
| _, err := registeredWorkflow.WrappedFunction(ctx, input, WithWorkflowID(workflow.id)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's keep the wrapped function private -- no need to expose it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated!
dbos/workflow.go
Outdated
|
|
||
| // ListRegisteredWorkflows returns information about workflows registered with DBOS. | ||
| // Each WorkflowRegistryEntry contains: | ||
| // - WrappedFunction: The underlying workflow implementation |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: update go doc too
Adds exported methods to list registered workflows and scheduled workflows with their registration parameters.
Resolves #150