fix: workflow output path schedule mode #745
Conversation
drockparashar
requested review from
firecast,
inishchith and
AtMrun
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR fixes inconsistent workflow output paths between manual and scheduled workflow runs by sanitizing the workflow ID to remove timestamp suffixes.
- Adds regex-based sanitization to remove timestamp suffixes from workflow IDs
- Ensures consistent output path format:
artifacts/apps/{application_name}/workflows/{workflow_id}/{run_id} - Imports the
remodule to support regular expression operations
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| # If workflow_id contains a timestamp (e.g., '-YYYY-MM-DDTHH:MM:SSZ'), remove it | ||
| sanitized_workflow_id = re.sub(r'-\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$', '', raw_workflow_id) |
There was a problem hiding this comment.
The regex pattern is hardcoded and assumes a specific timestamp format. Consider defining this as a module-level constant with a descriptive name like SCHEDULE_TIMESTAMP_PATTERN to improve maintainability and make the pattern reusable.
Copilot uses AI. Check for mistakes.
| raw_workflow_id = get_workflow_id() | ||
|
|
||
| # If workflow_id contains a timestamp (e.g., '-YYYY-MM-DDTHH:MM:SSZ'), remove it | ||
| sanitized_workflow_id = re.sub(r'-\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$', '', raw_workflow_id) |
There was a problem hiding this comment.
The regex should be compiled once at module level for better performance, especially since this function may be called frequently. Use re.compile() to create a compiled pattern object.
Copilot uses AI. Check for mistakes.
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
- Move timestamp regex to module-level constant for maintainability - Use compiled regex pattern for performance - Add fallback to raw workflow_id if sanitization results in empty string
…ub.com/drockparashar/application-sdk into fix/workflow-output-path-schedule-mode
| logger = get_logger(__name__) | ||
|
|
||
| # Compiled regex pattern for removing timestamp suffix from workflow IDs | ||
| TIMESTAMP_PATTERN = re.compile(r"-\d{4}-\d{2}-\d{2}T\d{2}:\d{2}:\d{2}Z$") |
There was a problem hiding this comment.
let's keep this variable localized as it's only used by a a single method as of now.
There was a problem hiding this comment.
I implemented that initially, but the copilot suggested me otherwise, what should i go with?
| if not sanitized_workflow_id: | ||
| sanitized_workflow_id = raw_workflow_id |
There was a problem hiding this comment.
when could be this be the case? can you handle in tests if so
There was a problem hiding this comment.
This case should not occur in practice, as workflow IDs are always generated by Temporal and will include a valid identifier. The fallback is just a safeguard for unexpected input, but is not expected to be triggered in real workflows.
Sure, I'll start working on it right away! |
|
@inishchith , I've added the unit tests for the build_output_path function in the tests folder! |
Changelog
-Ensures consistent workflow output path for both manual and scheduled runs.
-Removes any timestamp or schedule-specific suffix from workflow_id when generating the output path.
-Output path now always follows the format: artifacts/apps/{application_name}/workflows/{workflow_id}/{run_id}.
Additional context (e.g. screenshots, logs, links)
closes #719
Checklist
Copyleft License Compliance
Note
Normalize workflow output paths by stripping timestamp suffixes from workflow IDs; add targeted unit tests.
application_sdk/activities/common/utils.py):build_output_path: Strip schedule timestamp suffix (-YYYY-MM-DDTHH:MM:SSZ) fromworkflow_idusing compiledTIMESTAMP_PATTERNbefore formatting output path.reimport andTIMESTAMP_PATTERNconstant.tests/unit/activities/common/test_utils.py):build_output_pathcovering standard ID, scheduled ID with timestamp removal, and empty ID handling.Written by Cursor Bugbot for commit c10ff41. This will update automatically on new commits. Configure here.