Skip to content
Snippets Groups Projects

Add metadata about the GitLab server to GraphQL

Merged Nick Thomas requested to merge (removed):56809-graphql-version-api into master

What does this MR do?

Implements a "metadata" graphql resource that duplicates the REST /version endpoint, and transforms the latter into a graphql client for the former

What are the relevant issue numbers?

Does this MR meet the acceptance criteria?

Related to #56809 (closed)

Edited by Nick Thomas

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Nick Thomas
  • Nick Thomas
  • Nick Thomas added 1 commit

    added 1 commit

    • 8d02feb3 - Reimplement the REST /version API as a GraphQL query

    Compare with previous version

  • Nick Thomas added 1 commit

    added 1 commit

    • 6543a68e - Add metadata about the GitLab server to GraphQL

    Compare with previous version

  • Nick Thomas
  • Nick Thomas
  • assigned to @reprazent

  • Author Contributor

    @reprazent do you mind looking at this and adding your thoughts sometime? I guess I'm interested in the code itself, and also the idea of making this the recommended thing to do when creating overlap with some part of the REST API in graphql.

    If you can pass it to @DouweM after you're done for further comment, that'd be great. I don't think we should merge it right now, so it's WIPped.

    Is graphql still behind a feature flag? That would be one reason why we couldn't merge something like this in %11.8

  • Is graphql still behind a feature flag? That would be one reason why we couldn't merge something like this in %11.8

    It is, but it's enabled by default. I don't know if that's enough to be able to merge it. But we could have a dependent feature flag for these migrations?

  • I guess I'm interested in the code itself, and also the idea of making this the recommended thing to do when creating overlap with some part of the REST API in graphql.

    I love the idea of recommending this, but I guess we'd need to add some helpers to facilitate migrating before we do.

  • @DouweM What are your thoughts?

  • assigned to @DouweM

  • Nick Thomas added 51 commits

    added 51 commits

    Compare with previous version

  • the idea of making this the recommended thing to do when creating overlap with some part of the REST API in graphql.

    @nick.thomas As we start adding GraphQL resources for new features, these will usually come with just the minimal set of fields required for that feature, and it may be a while until they cover the complete set of fields and params (for sorting, ordering, including/exluding certain fields) that the REST endpoint offers. Could we have a "hybrid" REST endpoint in cases like this, building on the GraphQL resource and merging a Grape entity into it?

    If we can, and if we get those helpers @reprazent is referring to, I think we should definitely start recommending this route.

  • Douwe Maan
  • Nick Thomas added 900 commits

    added 900 commits

    Compare with previous version

  • Nick Thomas changed milestone to %11.9

    changed milestone to %11.9

  • Nick Thomas added ~2975006 label and removed backstage [DEPRECATED] label

    added ~2975006 label and removed backstage [DEPRECATED] label

  • Nick Thomas added 244 commits

    added 244 commits

    Compare with previous version

  • Nick Thomas marked the checklist item Changelog entry added, if necessary as completed

    marked the checklist item Changelog entry added, if necessary as completed

  • Nick Thomas added 1 commit

    added 1 commit

    • 2ef7625c - Add metadata about the GitLab server to GraphQL

    Compare with previous version

  • Nick Thomas unmarked as a Work In Progress

    unmarked as a Work In Progress

  • Author Contributor

    Hmm, if we can still disable graphql, that means we can't move the code over unconditionally. Even if it's enabled by default.

    I've reworked this so we have a conditionally_graphql! method and include both implementations. This means we're still performing authorization in the REST API, which means we can sidestep the authorization concerns above.

    @DouweM I think that doing a graphql query and then merging some extra stuff in, while certainly possible, might not be the best approach in general. In particular, I worry about performance, but I guess we can see how it looks and behaves the first time we attempt it.

  • Nick Thomas
  • Nick Thomas added 1 commit

    added 1 commit

    • dd5576ab - Add metadata about the GitLab server to GraphQL

    Compare with previous version

  • Nick Thomas added 1 commit

    added 1 commit

    • 89895d7c - Add metadata about the GitLab server to GraphQL

    Compare with previous version

  • Author Contributor

    @DouweM back for another round. I'd like to get the graphql metadata endpoint itself in, as I know we have at least one MR that might be better placed putting its data thaere, so if you think we should split this into two MRs to facilitate this, just let me know!

    I've kind of sidestepped the authorization question, I think it is possible, but we can address it once graphql cannot be disabled. WDYT?

  • assigned to @DouweM

  • @reprazent Can you please give this and the outstanding discussions another look? Once you're happy, I'll gladly do my maintainer review and merge it :)

  • assigned to @reprazent

    • Resolved by Nick Thomas

      Should we add a graphql only request spec to spec/requests/api/graphql/metadata_query_spec.rb. I don't imagine it should validate much more than "not failing", but it could be extended for the metadata we'll be adding in the future.

  • Bob Van Landuyt
  • assigned to @reprazent

  • Nick Thomas added 910 commits

    added 910 commits

    Compare with previous version

  • Author Contributor

    OK @reprazent, I've rebased and added the spec. Anything else you think needs doing in this MR?

  • Bob Van Landuyt approved this merge request

    approved this merge request

  • Nope, I don't think so.

    Only discussion remaining is whether or not we expose "Global" permissions on the metadata-type. But let's have @DouweM weigh in on that.

  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Loading
  • Please register or sign in to reply
    Loading