graph-node
graph-node copied to clipboard
all: GraphQL API Versioning
This is the initial implementation of versioning.
- [ ] remove the example implementation of a feature flag
I left few comments on the interesting parts of the code to make it easier to review.
- Where do we manage versions
- How do we define "latest" version
- How does the Gateway fetch a list of supported versions?
- How do we define feature flags?
- How do we validate the version number provided by the user?
Continues #3155
@kamilkisiela is this ready to review? I see that the CI is failing.
@evaporei yes, the code won't change much even after a rebase. I've got some questions both in code and gh comments, because I'm not sure if it's the right implementation.
hey @kamilkisiela have you got further updates here? just checking that you are not blocked
@azf20 no updates, I'm trying to implement support for interfaces in #3184 and once it's ready, I'll apply David's comments here.
@lutter ready for a review again, I implemented everything you asked for.
Those two failing tests (just the order of fields) are related to the fact I moved the ordering logic behind a feature flag to show what the versioning will look like. Once I remove the example feature flag, it will be fine.
@azf20 I read the GIP and applied some changes to the PR. When we get a version range, the semver package resolves to the most recent applicable version.
@lutter I did a refactor, it includes your suggestions.
Thanks @kamilkisiela and @lutter! I think starting with 1.0.0 is ok, but also interested in @That3Percent's expectations of a 1.x release
I think starting with 1.0.0 is ok, but also interested in @That3Percent's expectations of a 1.x release
I also think a 1.0.0 release is a good choice. Even if there are changes we know we would like to make, that's always the case so it's better to have a 1.0 and 2.0 than 0.x in perpetuity.
Agree that this approach seems fine, and I think the GIP already allows for that kind of minimum version specification
Hey @kamilkisiela I think this is good to merge, though looks like there are a couple of conflicts first! cc @lutter