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

Feature Management and Validation #2682

Merged
merged 56 commits into from
Aug 26, 2021
Merged

Feature Management and Validation #2682

merged 56 commits into from
Aug 26, 2021

Conversation

tilacog
Copy link
Contributor

@tilacog tilacog commented Aug 4, 2021

This PR adds:

  1. a new subgraph manifest validation step which checks that no undeclared feature is in use;
  2. a new graphQL endpoint that receives a subgraph Qm-hash and returns a list of its detected features or a list of undeclared features.

TODO:

  • Condition feature validation to a specific specVersion, I'll do that now.
  • Should we remove/refactor any code that dealt with feature management before, such as this?
  • Unit tests for serialization and deserialization of the SubgraphFeatures enum using camelCase names.
  • Integration tests
    • Validation on deployment
      • grafting
      • nonFatalErrors
      • fullTextSearch
      • IpfsOnEthereumContracts
    • GraphQL Endpoint
  • User-friendly messages on the graph-cli side

Fixes: #2715

@leoyvens leoyvens self-requested a review August 4, 2021 13:23
@tilacog
Copy link
Contributor Author

tilacog commented Aug 4, 2021

@leoyvens, two things:

  1. I forgot to condition feature validation to a specific specVersion, I'll do that now.
  2. I'm not sure if we should remove/refactor any code that dealt with feature management before, such as this.

I'll update the PR description to track this

@leoyvens
Copy link
Collaborator

leoyvens commented Aug 5, 2021

@tilacog Is this missing tests? What will be the testing strategy for this? I think unit tests might work fine.

@tilacog
Copy link
Contributor Author

tilacog commented Aug 5, 2021

Yes, there should be tests.

I think we could reuse the subgraphs we already have defined for integration tests here... and even writing new subgraphs to test new features as well.

@leoyvens
Copy link
Collaborator

leoyvens commented Aug 5, 2021

@tilacog What about the error message returned to graph-cli? We should make sure an informative message is being returned by the deploy json-rpc and is being printed by graph-cli.

@tilacog
Copy link
Contributor Author

tilacog commented Aug 5, 2021

@tilacog What about the error message returned to graph-cli? We should make sure an informative message is being returned by the deploy json-rpc and is being printed by graph-cli.

I'll link a companion PR for graph-cli in this PR's description, and vice-versa.

@tilacog tilacog force-pushed the tiago/feature-management branch 2 times, most recently from 7bd31d5 to 289b3a7 Compare August 12, 2021 15:51
@tilacog tilacog requested a review from leoyvens August 13, 2021 17:54
@evaporei evaporei self-requested a review August 18, 2021 20:20
@tilacog tilacog requested a review from evaporei August 19, 2021 16:40
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.

Feature management: identify "features" used by a subgraph
3 participants