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

Use UI Server GraphQL endpoint for suites #72

Merged
merged 1 commit into from May 26, 2019
Merged

Conversation

kinow
Copy link
Member

@kinow kinow commented May 21, 2019

Close #71

In the past during our first experiments with GraphQL, the endpoint was in cylc/cylc-flow. Now it resides in cylc/cylc-uiserver. The cylc scan was executed actually in a REST endpoint, that was executing the process locally in Python.

So this pull request removes the need to maintain that REST endpoint in cylc-uiserver (follow up PR after this one), and changes the Vue.js app to send a GraphQL query to the UI Server instead. The query used is:

query {
  tasks {
    id, name, meanElapsedTime, namespace, depth
  }
}

This change is based on the pull request by @dwsutherland to cylc-uiserver, which is still pending review. And while I believe we may have code changes, the data model proposed by @dwsutherland has received little negative feedback. So I believe it is safe now to start work on the Vue.js app based on that data model - furthermore, once we are pointing GraphQL queries to the right endpoint, changing the query and model is a 5 minutes job.

@kinow kinow self-assigned this May 21, 2019
@kinow kinow requested a review from dwsutherland May 21, 2019 00:43
@kinow
Copy link
Member Author

kinow commented May 21, 2019

@dwsutherland I believe you have not had the chance to write JS code for Vue.js app. But I think you are the best person to review this PR, as you will understand where we are coming from for these changes. I will annotate the pull request with some comments that may be helpful 👍

If you would like, you can also start cylc/cylc-uiserver with your GraphQL work, and use this branch to confirm the GraphQL query goes through instead of a REST request.

Below my verification that it worked locally.

image

@codecov-io
Copy link

codecov-io commented May 21, 2019

Codecov Report

Merging #72 into master will not change coverage.
The diff coverage is 92.3%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master      #72   +/-   ##
=======================================
  Coverage   78.22%   78.22%           
=======================================
  Files          13       13           
  Lines         124      124           
=======================================
  Hits           97       97           
  Misses         27       27
Impacted Files Coverage Δ
src/model/Suite.model.js 100% <100%> (ø) ⬆️
src/services/suite.service.js 93.75% <88.88%> (-0.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d9c6395...be71dc1. Read the comment docs.

this.name = name;
this.user = user;
this.owner = owner;
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why but I used user for a Suite (which will be renamed to Workflow in another change #73 ).

workflows {
id, name, owner, host, port
}
}
Copy link
Member Author

Choose a reason for hiding this comment

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

The GraphQL query. gql is a special syntax which some day I will distill to understand how that works in JavaScript. But that comes from this apollo library to parse graphql queries.

createGraphqlClient(host, port) {
return createApolloClient(`http://${host}:${port}/graphql`);
createGraphqlClient() {
return createApolloClient(`${window.location.pathname}/graphql`);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here we were sending requests to whatever cylc scan returned for host:port. That's because we were sending the queries to cylc/cylc-flow. Instead now we copy the URL + /graphql, which should match the Tornado Handler for GraphQL.

return store.dispatch('suites/setSuites', suites);
}).catch((error) => {
const alert = new Alert(error.message, null, 'error');
return store.dispatch('addAlert', alert);
Copy link
Member Author

Choose a reason for hiding this comment

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

Here is the change to query from GraphQL, removing the query to REST (I copied the existing call to retrieve tasks, which is broken, but will be fixed in #70 )

getSuiteTasks(suite) {
const apolloClient = this.createGraphqlClient(suite.host, suite.port);
getSuiteTasks() {
const apolloClient = this.createGraphqlClient();
Copy link
Member Author

Choose a reason for hiding this comment

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

No more need for information from the suite, we know we will talk to the UI Server used by this Vue.js app, not to a Cylc Workflow Service anymore.

};
sandbox.stub(axios, 'get').rejects(e);
sandbox.stub(SuiteService, 'createGraphqlClient').returns(stubClient);
Copy link
Member Author

Choose a reason for hiding this comment

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

The rest here are just changes for tests to match what was changed before 👍 hope that helps you reviewing this.

@dwsutherland
Copy link
Member

dwsutherland commented May 21, 2019

Yes, use the workflows query as the cylc scan... and you can always include tasks like this:

query {
  workflows {
    id
    name
    owner
    host
    port
    stateTotals
    tasks {
      id
      name
      proxies {
        cyclePoint
        state
        meanElapsedTime
        namespace
      }
    }
  }
}

Copy link
Member

@dwsutherland dwsutherland left a comment

Choose a reason for hiding this comment

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

LGTM at a glance XD

@kinow
Copy link
Member Author

kinow commented May 26, 2019

One review ruling for now :) merging it 👍

@kinow kinow merged commit cb9538c into cylc:master May 26, 2019
@kinow kinow added this to the 0.1 milestone Sep 10, 2019
@kinow kinow deleted the graphql-suites branch October 30, 2019 02:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Query workflows/suites from GraphQL instead of REST endpoint
3 participants