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

Add the ability to use environment variables as HTTP headers when introspecting schemas, etc #67

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

jakemwood
Copy link

In our workflow, we are finding ourselves needing to include authorization tokens in the graphql-config.yml file, which can be a security risk since you may accidentally check a token into source control.

This PR allows a user to set an environment variable, called TURMS_HTTP_HEADERS, which would be a JSON object containing the headers they'd like to send along with their requests to their GraphQL server.

You can find examples in the unit tests. I wasn't sure where/if I should document this feature, since there are other environment variables in use that do not seem to be documented in the readme. Happy to document this behavior elsewhere. I also wasn't sure how to format the project, I tried running black but many other changes came along with it. To keep the PR size minimal, I have excluded formatting.

Thanks for your consideration!

@jhnnsrs
Copy link
Owner

jhnnsrs commented Aug 21, 2023

Hi thanks a lot for the PR!

I really need to take some time to update the readme now that the API and configuration format has changed.

And totally agreed that there should be some way of passing additional headers through env variables. I am just wondering if it isbworh keeping the env logic inside the project configuration which should stand as the source of truth for the whole generation? What do you think?

Maybe including an additional headers setting in the config, that has an environmental overwrite?

@enkoder
Copy link

enkoder commented Feb 27, 2024

@jhnnsrs yes please! This feature is super important as I'm trying to work behind a private api

@deyton
Copy link

deyton commented Jul 12, 2024

Hi thanks a lot for the PR!

I really need to take some time to update the readme now that the API and configuration format has changed.

And totally agreed that there should be some way of passing additional headers through env variables. I am just wondering if it isbworh keeping the env logic inside the project configuration which should stand as the source of truth for the whole generation? What do you think?

Maybe including an additional headers setting in the config, that has an environmental overwrite?

hey @jhnnsrs, can you elaborate on how we could keep the env logic inside configuration file? I don't think I understand what it means. Do you mean having a config that specifies which environment variable to look in for header overrides? That way it would be clearer that the headers specified in the config file may not be the ones used. I can update this PR with whatever suggestions you have to make it better 😄

Thanks!

@jhnnsrs
Copy link
Owner

jhnnsrs commented Jul 14, 2024

Hi everyone, sorry wasn't planning on delaying this indefinetely,
I was just hopping that we could use the Project Config BaseModel that is
anyways inheriting from BaseSettings with the env_prefix settings according
to: https://docs.pydantic.dev/latest/concepts/pydantic_settings/#usage

Just looks potentially cleaner and would allow us to come up and inline document
some other environment variables? Generally also happy to include this and then
change strategy later if it is pressing?

Sorry for the holdup.

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.

4 participants