-
Notifications
You must be signed in to change notification settings - Fork 0
Support retrieval of temporary published outputs #146
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
Conversation
Anticipating having this function deligate depending on the output type
This lets us more clearly specify which output type is used in a recipe
6fafb9d to
71bc4dd
Compare
I could have sworn this was the default behavior...?
59bf0fe to
718897b
Compare
For now we can just always assume a recipe with a temporary output has not been published.
227e98a to
9401699
Compare
| input: RecipeInput | ||
| output: RecipeOutput = RecipeOutput() | ||
| output: DataOneRecipeOutput | TemporaryRecipeOutput | PvcRecipeOutput = ( | ||
| PvcRecipeOutput() |
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.
Default to PVC output, which was the only and default behavior prior to this PR. This makes it a little more explicit/clear. But do we want this as the default? Maybe temporary output makes more sense as a default?
| # location uses a key that's based on the argo workflow name. We could | ||
| # probably use `hera` to filter for a matching workflow based on the | ||
| # `recipe_config.id`, and check its output, but this will be much easier | ||
| # to implement once we track recipe executions in the database. |
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.
Related issue: #148
This should be temporary. Need to address the db component to track recipe executions.
b6dc302 to
d102260
Compare
This is required so that swagger knows it lives under a root path when behind ingress/proxy
6338f36 to
882435d
Compare
rushirajnenuji
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.
Reviewed and tested this locally, and it looks good. All my tests passed locally. 🚀
One thing that we should add in the future iterations is the workflow ownership checks before retrieving the temp output artifacts generated by the workflow.
We can go ahead and merge this PR, thank you @trey-stafford for adding this support. 🎉🚀
Adds a new endpoint and CLI for downloading the results of a workflow with a
temporaryoutput type. Resolves #110.See also:
outputcfg perogdc-runner#146 ogdc-recipes#10Prior to this PR, final outputs would be placed in a PVC, and these were not particularly accessible without direct access to the PVC. Now there is a new output type,
temporary, which zips the final output of a recipe and places it Argo's configured artifact storage (s3). The new service endpoint takes a workflow name and returns a pre-signed URL to the object that users can use to download the data.The pre-signed url allows access to the data object for 2 hours, and then the URL expires.
The idea here is that workflows with temporary outputs can be cleaned up after a given amount of time. This works well for recipes that do not need to formally publish outputs (e.g,. a subsetting operation).
Along with the PRs mentioned above, the API landing page should not be accessible at: https://localhost:7443/ogdc/api . Note that you will need to accept a security exception, as this HTTPS endpoint utilizes a self-signed (unverified) cert.