Skip to content
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

api: add seeding endpoint #11

Merged
merged 2 commits into from
Oct 19, 2017

Conversation

diegodelemos
Copy link
Member

@diegodelemos diegodelemos commented Oct 6, 2017

Signed-off-by: Diego Rodriguez diego.rodriguez@cern.ch

@diegodelemos diegodelemos self-assigned this Oct 6, 2017
@diegodelemos diegodelemos added this to the Client-Server milestone Oct 6, 2017
@diegodelemos diegodelemos force-pushed the analysis-seeding branch 2 times, most recently from 0b6bd02 to 55d43b2 Compare October 12, 2017 15:09
Copy link
Contributor

@hjhsalo hjhsalo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some comments of style and language used. Nothing that prevents merging so, LGTM

Disclaimer: Just reviewed code. Didn't test with live REANA Cluster.

post:
summary: Seeds the analysis workspace with the provided file.
description: >-
This resource expects a file to be placed in the analysis workspace
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe

This resource expects a file that is to be placed in the analysis workspace identified by the UUID analysis_id.

(to be placed --> is to be placed)

@hjhsalo
Copy link
Contributor

hjhsalo commented Oct 18, 2017

This is not really part of the PR but in reana_server/api_client.py#L35 there is:

spec_file_path = os.path.join(
    pkg_resources.resource_filename('reana_server',
                                    'openapi_connections'),
  spec_file)

It is PEP8-compliant, but I missed the spec_file part while skimming through the file so maybe it should changed a bit to improve readibility?
Something like this perhaps?:

spec_file_path = os.path.join(
                    pkg_resources.
                    resource_filename(
                        'reana_server',
                        'openapi_connections'),
                    spec_file)

or

spec_file_path = os.path.join(
            pkg_resources.
            resource_filename(
                'reana_server',
                'openapi_connections'),
            spec_file)

@hjhsalo
Copy link
Contributor

hjhsalo commented Oct 18, 2017

Maybe change this in reana_server/rest/analyses.py#L115:

This resource is expecting JSON data with all the necessary
information to instantiate a yadage workflow.

(informations --> information)

@hjhsalo
Copy link
Contributor

hjhsalo commented Oct 18, 2017

In reana_server/rest/analyses.py#L33:

@blueprint.route('/analyses', methods=['GET'])
def get_analyses():  # noqa
    r"""Get all analyses.
    ---
    get:
      summary: Returns list of all analyses.
      description: >-
        This resource return all analyses in JSON format.

Term all analyses is not very clear.
But to be honest, I don't know if this is any better:
Returns list of all analyses (running, finished, waiting) in REANA cluster.

Or maybe it can be opened in the description part just below this one.

Are there more states for an analysis than running, finished, waiting?

Diego Rodriguez added 2 commits October 19, 2017 07:49
* Adds a redundant field `file_name` since bravado client doesn't
  propagate the file name. Waiting for Yelp/bravado-core#201 to be
  implemented.

Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
Signed-off-by: Diego Rodriguez <diego.rodriguez@cern.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants