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

feat(link): add proto_paths support #3222

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DLillard0
Copy link
Contributor

Summary:
Previously, this pr[3206] only processed the proto_paths during gen, but when using tailcall start, there will still be path problems, so it is necessary to save the proto_paths information on the link to ensure that the path can be processed correctly during tailcall start.

Config:

{
  "inputs": [
    {
      "proto": {
        "src": "news.proto",
        "url": "http://localhost:50051",
        "protoPaths": ["tailcall-fixtures/fixtures/protobuf"]
      }
    }
  ],

  ...
}

Output:

schema
  @server
  @upstream
  @link(
    src: "news.proto"
    # new attr
    proto_paths: ["tailcall-fixtures/fixtures/protobuf"]
    type: Protobuf
  ) {
  query: Query
}
...

Issue Reference(s):
None

Build & Testing:

  • I ran cargo test successfully.
  • I have run ./lint.sh --mode=fix to fix all linting issues raised by ./lint.sh --mode=check.

Checklist:

  • I have added relevant unit & integration tests.
  • I have updated the documentation accordingly.
  • I have performed a self-review of my code.
  • PR follows the naming convention of <type>(<optional scope>): <title>

@github-actions github-actions bot added the type: feature Brand new functionality, features, pages, workflows, endpoints, etc. label Dec 14, 2024
@DLillard0 DLillard0 force-pushed the feature/link-directive-support-proto-paths branch 2 times, most recently from 68fdb58 to 2ab74f0 Compare December 14, 2024 06:01
@tusharmath
Copy link
Contributor

@DLillard0 Thanks for the PR! Our goal ultimately is to drop link from the GraphQL configuration. I have talked about it in detail over here - https://tailcall.run/blog/migrating-to-graphql-configuration-v2/

We basically two options now:

  1. Generate a file with just the schema information without any links.
  2. Generate two files: a main.graphql and a main.yml (for links).

I prefer Option 1 because its a lot simpler to work with. Let me know what you think?

@DLillard0
Copy link
Contributor Author

@tusharmath I also agree with Option 1. If there is a real need for modular configuration management, can we also implement the current link functionality by providing an additional library? This will decouple it from the current library and reduce the current complexity.
Is there a timeline for removing the link from the GraphQL configuration?

@tusharmath
Copy link
Contributor

tusharmath commented Dec 15, 2024

I also agree with Option 1. If there is a real need for modular configuration management, can we also implement the current link functionality by providing an additional library?

It's always nice to organize configurations into smaller files. In large organizations, these files are owned and maintained by individual teams. Link functionality still exists however it is only available in the YML/JSON format.

Is there a timeline for removing the link from the GraphQL configuration?

This feature has already been shipped, you can try it out.

@DLillard0
Copy link
Contributor Author

@tusharmath I have two more questions.

  1. Is there any way to generate a YML/JSON configuration file? like tailcall gen?
  2. Moving the Link functionality to the configuration file also seems to require dealing with the path problem of GraphQL dependencies. It seems that Link still needs to support proto_paths.

@tusharmath
Copy link
Contributor

Is there any way to generate a YML/JSON configuration file? like tailcall gen?

Not via tailcall gen. But I don't think these are very complicated to hand-write.

Moving the Link functionality to the configuration file also seems to require dealing with the path problem of GraphQL dependencies. It seems that Link still needs to support proto_paths.

You are right, we need to still update config::link.rs to support proto_path. However, I think the generated file should not contain any @link directives.

@DLillard0
Copy link
Contributor Author

Thanks for your answer!

@DLillard0 DLillard0 force-pushed the feature/link-directive-support-proto-paths branch from 2ab74f0 to e6ea273 Compare December 16, 2024 08:21
Copy link

codecov bot commented Dec 16, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.41%. Comparing base (76fc565) to head (f4496e6).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3222      +/-   ##
==========================================
+ Coverage   86.39%   86.41%   +0.02%     
==========================================
  Files         282      282              
  Lines       28724    28773      +49     
==========================================
+ Hits        24816    24865      +49     
  Misses       3908     3908              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@DLillard0 DLillard0 force-pushed the feature/link-directive-support-proto-paths branch from e6ea273 to 6b0219d Compare December 16, 2024 09:40
@DLillard0 DLillard0 force-pushed the feature/link-directive-support-proto-paths branch from 6b0219d to f4496e6 Compare December 16, 2024 09:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
type: feature Brand new functionality, features, pages, workflows, endpoints, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants