api: add set_workflow_status function #50
Conversation
anton-khodak
changed the title
| organization = request.args['organization'] | ||
| user_uuid = request.args['user'] | ||
| workflow = Workflow.query.filter(Workflow.id_ == workflow_id).first() | ||
| status = request.json |
There was a problem hiding this comment.
We could check here if the status sent is correct and send a 400 if it is not. You have three variables representing status on the top of the file, you could make a list with the values and check the incoming string belongs to it, what do you think?
Also, did you explore the possibility of adding new tests to check this scenarios?
There was a problem hiding this comment.
I agree, enumeration checks and tests are necessary
reana_workflow_controller/rest.py
Outdated
| {'message': 'User {} is not allowed to access workflow {}' | ||
| .format(user_uuid, workflow_id)}), 403 | ||
| if status == 'start': | ||
| return start_workflow(organization, user_uuid, workflow) | ||
| return jsonify({'id': workflow.id_, | ||
| 'status': workflow.status.name, |
There was a problem hiding this comment.
This status will be the previous (the one you just got from DB), one would expect to retrieve the requested status. I would return, for now, the requested status as it is (as long as it is correct).
I know it is misleading (if you want to start, it could perfectly happen that it fails) but since we are calling a celery task which doesn't return any value and until this gets executed in the workflow engine there won't be any change in DB we can't do anything else but returning the value as it is.
There was a problem hiding this comment.
I will return the requested status for now, but we could extend the set of statuses and return here "starting" or "pending", for example, and "started" when the workflow starts in the workflow engine.
reana_workflow_controller/rest.py
Outdated
| kwargs = { | ||
| "workflow_workspace": workflow.workspace_path, | ||
| "workflow_json": workflow.specification, | ||
| "parameters": workflow.parameters |
There was a problem hiding this comment.
workflow_uuid is missing see here. This will probably break when calling celery, have you tried to do the following with all your PRs integrated:
~/reana-demo-root6-roofit $ reana-client -l debug workflow create -f .reana.yaml
some-uuid
~/reana-demo-root6-roofit $ reana-client workflow start --workflow some-uuid
workflow started blah blahAnd check if it actually works?
There was a problem hiding this comment.
Indeed, I just copied the code from run_yadage_workflow_from_spec_endpoint, it is fixed. However, I have troubles with spawning yadage workers both without kubernetes and inside minikube, so I couldn't test the full chain.
(the trouble is in connecting to message broker, I have UnicodeDecodeError, would be good if you could verify running/failing engine-yadage later. Engine-cwl launches correclty)
| @@ -43,6 +48,10 @@ | |||
| 'default': 'default-queue' | |||
| } | |||
|
|
|||
| workflow_spec_to_task = { | |||
| .format(user_uuid, workflow_id)}), 403 | ||
| if status == START: | ||
| return start_workflow(organization, user_uuid, workflow) | ||
| elif status == STOP: |
There was a problem hiding this comment.
This is not implemented yet. I would remove it since changing the status in DB won't do anything, if the workflow is running it will keep running anyways and user will see finished status while it is not true.
There was a problem hiding this comment.
I created it to test status changes similar to get_workflow_status tests. If stop is not on your roadmap in the nearest future, I will remove it.
There was a problem hiding this comment.
Indeed it is in the roadmap, but still not implemented so this "changing in the DB directly" would be kind of a "lie" because it is not how it will be implemented most probably.
If you want to check status changes you can get the status right after creating the workflow, which should be created, and then submit status change for started so you check if it works.
| pass | ||
|
|
||
|
|
||
| def stop_workflow(organization, user_uuid, workflow): |
There was a problem hiding this comment.
Not implemented, and probably it won't be the way of stopping it (through DB)
|
once amended those small changes, could you please squash the commits since everything belongs to the same functionality added? then we |
|
I couldn't figure out how to squash the commits with a prior merge commit, so I am switching to a new clean branch #51 |
No description provided.