-
Notifications
You must be signed in to change notification settings - Fork 244
Update to connexion 3 #5268
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
Update to connexion 3 #5268
Conversation
adamnovak
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.
This looks pretty good, but I think we should keep using secure_path() if we can, and I don't see a good reason why we can't.
| file.save(dest) | ||
| has_attachments = True | ||
| body["workflow_attachment"] = ( | ||
| "file://%s" % temp_dir |
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.
There might be encoding issues here if the temp directory has special characters in it, but they're not new to this PR.
Co-authored-by: Adam Novak <[email protected]>
adamnovak
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.
I think this is good now.
|
|
This should resolve #4927 once common-workflow-language/workflow-service#136 is merged into workflow-service.
Changelog Entry
To be copied to the draft changelog by merger:
Reviewer Checklist
issues/XXXX-fix-the-thingin the Toil repo, or from an external repo.camelCasethat want to be insnake_case.docs/running/{cliOptions,cwl,wdl}.rstMerger Checklist