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

V2 graphql #514

Merged
merged 5 commits into from
May 16, 2022
Merged

V2 graphql #514

merged 5 commits into from
May 16, 2022

Conversation

JPrevost
Copy link
Member

This forks the Search model to OpenSearch and connects GraphQL to use that new OpenSearch model.

Please see the commit message for the most recent commit for much more context and side effects of this work.

How can a reviewer manually see the effects of these changes?

  • Using the built in GrapiQL playground, note the deprecated fields and how they can still be used but throw warnings.
  • Retrieve new fields and deprecated fields

What are the relevant tickets?

Requires Database Migrations?

NO

Includes new or updated dependencies?

YES

@jazairi jazairi self-assigned this May 16, 2022
Copy link
Contributor

@jazairi jazairi left a comment

Choose a reason for hiding this comment

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

I won't pretend to grok every detail of this, but based on my understanding of the change set, this seems like a valid solution. I think that once we load the legacy data, we should plan to remove the FlipFlop feature and V1 stuff (assuming I'm correctly interpreting that that will be possible). Aside from making the test suite easier to run, it should make this more maintainable in the long run. Based on my experience doing something similar with Bento, it shouldn't be too painful.

Thanks for taking this on and sharing the context around it. :shipit:

@jazairi
Copy link
Contributor

jazairi commented May 16, 2022

I should add, the test coverage drop doesn't concern me, since most if not all of it seems to be related to the deprecations and V1 side of conditionals, both of which will hopefully be dropped when we remove this in the future. Likewise, many of the flags CodeClimate raised will be lowered as a result of that same maintenance.

Why are these changes being introduced:

* we are moving to GraphQL as our sole endpoint
* we are using OpenSearch as our backend

Relevant ticket(s):

* https://mitlibraries.atlassian.net/browse/RDI-101

How does this address that need:

* Adds new v2 fields to GraphQL Records
* Deprecates fields by both documenting them as
  deprecated and which field to use instead, and also copies the data
  to the old field when possible
* Updates CI to run both OpenSearch and ElasticSearch versions of the
  tests

Document any side effects to this change:

note: filtering is mostly broken still at the Opensearch model level

note: Flipflip was used as it makes it slightly easier to flip states
in testing. I considered just using ENV but it was a bit clunky.

note: a few fields were unable to be deprecated cleanly. We could
consider renaming the new fields to allow for deprecation.

The tests need to be run twice until a better solution is identified or
we remove the elasticsearch version of the GraphQL (which will be in
the next few months). This is caused by our GraphQL implementation only
loading the config the first time it is used. The effect is that if an
elasticsearch test is run first, all the OpenSearch tests fail or vice
versa. The workaround was to name all of the tests as `graphqlv1` or
`graphqlv2` and using a filter to exclude running them. CI will run both
versions of the tests separately. Locally, developers should choose to
run whichever makes the most sense for their context. I wasn't able to
come up with a great way to default to one locally yet.
@JPrevost JPrevost merged commit 40ca6e2 into main May 16, 2022
@JPrevost JPrevost deleted the v2_graphql branch May 16, 2022 21:04
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.

2 participants