Add metadata about the GitLab server to GraphQL
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?
-
Changelog entry added, if necessary -
Documentation created/updated via this MR -
Documentation reviewed by technical writer or follow-up review issue created -
Tests added for this feature/bug -
Tested in all supported browsers -
Conforms to the code review guidelines -
Conforms to the merge request performance guidelines -
Conforms to the style guides -
Conforms to the database guides -
Link to e2e tests MR added if this MR has Requires e2e tests label. See the Test Planning Process. -
Security reports checked/validated by reviewer
Related to #56809 (closed)
Merge request reports
Activity
added GraphQL backstage [DEPRECATED] labels
mentioned in issue #56809 (closed)
- Resolved by Nick Thomas
- Resolved by Douwe Maan
- Resolved by Nick Thomas
- Resolved by Nick Thomas
- Resolved by Nick Thomas
added 1 commit
- 8d02feb3 - Reimplement the REST /version API as a GraphQL query
added 1 commit
- 6543a68e - Add metadata about the GitLab server to GraphQL
- Resolved by Nick Thomas
- Resolved by Douwe Maan
assigned to @reprazent
@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
- Resolved by Nick Thomas
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?
@DouweM What are your thoughts?
assigned to @DouweM
added 51 commits
-
6543a68e...47e6675c - 50 commits from branch
gitlab-org:master
- 415f5106 - Add metadata about the GitLab server to GraphQL
-
6543a68e...47e6675c - 50 commits from branch
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.
- Resolved by Nick Thomas
assigned to @nick.thomas
added 900 commits
-
415f5106...a1215556 - 899 commits from branch
gitlab-org:master
- afaf7298 - Add metadata about the GitLab server to GraphQL
-
415f5106...a1215556 - 899 commits from branch
changed milestone to %11.9
added ~2975006 label and removed backstage [DEPRECATED] label
added 244 commits
-
afaf7298...ab06c9b6 - 243 commits from branch
gitlab-org:master
- 0bcbc5d0 - Add metadata about the GitLab server to GraphQL
-
afaf7298...ab06c9b6 - 243 commits from branch
marked the checklist item Changelog entry added, if necessary as completed
added 1 commit
- 2ef7625c - Add metadata about the GitLab server to GraphQL
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.
- Resolved by Nick Thomas
added 1 commit
- dd5576ab - Add metadata about the GitLab server to GraphQL
- Resolved by Nick Thomas
added 1 commit
- 89895d7c - Add metadata about the GitLab server to GraphQL
@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 Bob Van Landuyt
- 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.
assigned to @nick.thomas
- Resolved by Douwe Maan
mentioned in issue gitlab-com/www-gitlab-com#3350 (closed)
assigned to @reprazent
added 910 commits
-
89895d7c...83cb7482 - 909 commits from branch
gitlab-org:master
- 5d7fb053 - Add metadata about the GitLab server to GraphQL
-
89895d7c...83cb7482 - 909 commits from branch
OK @reprazent, I've rebased and added the spec. Anything else you think needs doing in this MR?
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.