chore(ux): better handle uploading of workflow #1907
Conversation
|
@rajeshj11 is attempting to deploy a commit to the KeepHQ Team on Vercel. A member of the Team first needs to authorize it. |
|
@shahargl please review |
…jeshj11/keep into feat-1885-workflow-validators-enhance
…jeshj11/keep into feat-1885-workflow-validators-enhance
keep/parser/parser.py
Outdated
|
|
||
| # get the workflow id from the database | ||
| workflow_id = get_workflow_id(tenant_id, workflow_name) | ||
| workflow_id = get_workflow_id(tenant_id, workflow.get("id")) |
| # if the workflow id is not found, it means that the workflow is not stored in the db | ||
| # for example when running from CLI | ||
| # so for backward compatibility, we will use the workflow name as the id | ||
| # todo - refactor CLI to use db also | ||
| if not workflow_id: |
| interval = self.parser.parse_interval(workflow) | ||
| if not workflow.get("name"): # workflow name is None or empty string | ||
| workflow_name = workflow_id | ||
| workflow_name = workflow_id if self._is_not_uuid(workflow_id) else "[No Workflow Name]" |
|
@rajeshj11 hey, left few comments. IMO its not ready. also - please add tests so this change will be testable. the best way is to think of the tests, make sure they are failing before your change, and show that your changes make them pass. |
ack |
…ring based on feedback
…jeshj11/keep into feat-1885-workflow-validators-enhance
Signed-off-by: Rajesh Jonnalagadda <[email protected]>
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
| interval = self.parser.parse_interval(workflow) | ||
| if not workflow.get("name"): # workflow name is None or empty string | ||
| workflow_name = workflow_id | ||
| workflow_name = workflow_id if workflow_id and self._is_not_uuid(workflow_id) else "[No Workflow Name]" |
There was a problem hiding this comment.
workflow cannot be created without name
| @@ -306,24 +306,42 @@ def add_or_update_workflow( | |||
| description, | |||
| created_by, | |||
| interval, | |||
| workflow_raw, | |||
| update_raw, | |||
| provisioned=False, | ||
| provisioned_file=None, | ||
| re_provision=False, |
|
|
||
| if existing_workflow: | ||
| workflow_raw = update_raw(existing_workflow.id) |
| is_disabled, | ||
| workflow_id, |
There was a problem hiding this comment.
but it already get id, so what's workflow_id?
…jeshj11/keep into feat-1885-workflow-validators-enhance
…jeshj11/keep into feat-1885-workflow-validators-enhance
|
@rajeshj11 I appreciate your contribution! Unfortunately I see there are a lot of pieces hard for understand for me and for @shahargl . I will close the PR for now, please open another PR (or PRs) once it's ready for the review. |
I had previously requested a 1:1 call with @shahargl but didn’t receive a response. I respect your decision to close the PR and appreciate your feedback. However, after considering the changes, I believe the approach will remain consistent, even with additional adjustments. Please feel free to unassign me from this task if needed. |
rajesh-jonnalagadda
deleted the
feat-1885-workflow-validators-enhance
branch
…or at card level
Closes #1885
/claim #1885
📑 Description
Enhance workflow validators
Today "id" is being used as an name. This behavior should be deprecated. If an id is supplied, use it, and use name as name https://github.com/keephq/keep/blob/main/keep/parser/parser.py#L37
Deleting an workflow and creating another workflow with the same name shows again all old data since deleting a workflow make it is_deleted=1 and not really deletes the workflow (which is ok). Uploading a workflow without an id should generated different uuid.
Improve validation - mark workflow with some flag that says it won't run (e.g. configuration error) until something will fix it
✅ Checks
ℹ Additional Information
demo video
https://www.loom.com/share/9f234685255a4d79b6dc3adf279a8437