silverstripe / silverstripe-graphql

Serves Silverstripe data as GraphQL representations
BSD 3-Clause "New" or "Revised" License
52 stars 61 forks source link

Store GraphQL v5 schema in silverstripe-cache folder #529

Closed GuySartorelli closed 1 year ago

GuySartorelli commented 1 year ago

This has come up time and time again - most recently in https://github.com/silverstripe/silverstripe-graphql/issues/465

The main argument against it at the time was that we were very close to releasing the stable release of GraphQL v4 and we didn't want to poke the bear.

We now find ourselves in a situation where we're creating a new major release of this module, and we have the opportunity to reconsider what the default directory is for the graphql generated schema.

We've agreed internally that for CMS 5 the graphql schema should be included in the silverstripe-cache directory by default.

Acceptance criteria

Notes

PRs

michalkleiner commented 1 year ago

We are in feature freeze for CMS 5.0, right? Would this go against it if it was done for 5.0.0?

michalkleiner commented 1 year ago

If the overridden config still supports relative paths, then that will suffice for use cases where developers want to commit the generated files, but if it doesn't, we should still support the use case when the schema is generated e.g. by CI pipeline and deployed as part of the application.

GuySartorelli commented 1 year ago

We are in feature freeze for CMS 5.0, right? Would this go against it if it was done for 5.0.0?

My understanding is that we're in a "breaking API changes" freeze but new features are okay - but not 100% sure and the changelog section about it doesn't specify. @maxime-rainville what say you?

If the overridden config still supports relative paths

It will, don't worry. That config will work exactly the same way it currently does, with the exception that it's default value will be a blank string, and having it set to a blank string will result in storing in the cache dir.

GuySartorelli commented 1 year ago

For future people reading these comments: Max decided this didn't make the cut for CMS 5.0.0 but is worthwhile to add as an optional feature for 5.1.0

GuySartorelli commented 1 year ago

This has been moved to "awaiting" because we can't test in SC without deploying over what's currently there - and what's currently there is needed for an ongoing investigation about solr not working.

michalkleiner commented 1 year ago

Can you ask platform team to have an additional test environment added to the testing stack?

GuySartorelli commented 1 year ago

This has now been tested in cloud.

emteknetnz commented 1 year ago

Linked PRs have been merged and will be released in 5.1.0