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] Dockerfile #11

Open
2 of 6 tasks
j-riebe opened this issue Feb 28, 2022 · 14 comments
Open
2 of 6 tasks

[FEATURE] Dockerfile #11

j-riebe opened this issue Feb 28, 2022 · 14 comments

Comments

@j-riebe
Copy link
Contributor

j-riebe commented Feb 28, 2022

Openening a new Issue as this might grow... (#7)

Reference Dockerfile

FROM python:3.9-alpine  # small python base image

RUN pip install turms  # maybe fix version to image tag and include optional dependencies

VOLUME /graphql.config.yaml
VOLUME /generated
WORKDIR /

CMD ["turms", "gen"]

There are some things to notice here:

  • We need to mount the generated files as a volume, to get them outside of the docker container.
  • The location must match with the settings.
  • Mounting configuration files is pretty unintuitive when using Docker. Overwriting reasonable default via ENV vars should be preferred.

This leaves following points to look into:

  • Make configuration possible via ENV vars (see BaseSettings, @jhnnsrs you could do this best I guess)
  • Define (reasonable) default graphql.config.yaml
  • Create Dockerfile / docker-compose.yml example
  • Figure out how to mount custom processors into the container (maybe extend turms base image?)
  • Implement CI flow
  • Publish Dockerimage
@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

I could help you with the Docker related stuff, if you need additional help.

Don't know if it's worth a PR or if a comment to this issue does the job also 😄

The bigger fish is configuring everything without the need of mounting a custom config file.

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

Ok I got a bit into it.

You could build the current settings handling by the aforementioned BaseSettings and build one single settings object once you start the generator.

All plugin / processor / generator configs are then Part of the "superSettings". By doing so, we can overwrite every Single value with ENV vars (nested objects also).

You currently read the yml "by hand" for every plugin. This looks very similar to pydantic's TaggedUnion

This leads to sth.like

from pydantic import BaseSettings 

class GraphQLConfig(BaseSettings):
    generator_config: GeneratorConfig
    plugins: List[BasePluginConfig]
    processors: List[BaseProcessorConfig]
    ...

BTW this approach can use the same config file like now, because its possible to populate the pydantic settings directly from a yml file and overwrite certain values with ENV vars.

@jhnnsrs
Copy link
Owner

jhnnsrs commented Feb 28, 2022

Like the idea about BaseSettings approach in general! But I believe that it could get quite verbose and to nested easily? How should we deal with something like additional base classes for specific types? Also I am not quite sure how pydantic would handle serializing unions that would be loaded dynamically (the original reason why I opted against it). Any ideas? :)

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

For the dynamically loaded unions, see the TaggedUnions link above and set ...(discriminator='type')
This feature is quite new (end of 2021)

(And I edited the above comment, I know you're fast, but that was too fast 😄 )

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

The config is already nested, which is why yaml is a nice approach.

What I'm missing is that if you just want to change one field - say the schema url - you'd need to mount a complete config file because elemetwise override is currently not possible.

Pydantic also Supports exploding env var values that are provided as jsons into nested objects. This would allow

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

But I think I'm drifting away ...

My main concern is making the Dockerimage easy to use with setting as few values as possible.
This requires:

  • preventing misconfiguration (e.g. out_dir===/generated)
  • easy de-/activation of all available plugins/processors/stylers

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

And maybe you could consider a designated configuration file style, instead of trying to comply with the graphql-codegen style, when this library will never be an executable extension of the JS project.

@jhnnsrs
Copy link
Owner

jhnnsrs commented Feb 28, 2022

No no i like the drift :D Will explore tagged unions a bit more as they also seem to be the perfect fit for the graphql "__typename" discrimination. Noted for future!

Also ENV overwrites are a very good idea, maybe focussing on some sensible defaults first (SCHEMA_URL, GENERATED_PATH, etc), what do you think?

@jhnnsrs
Copy link
Owner

jhnnsrs commented Feb 28, 2022

And maybe you could consider a designated configuration file style, instead of trying to comply with the graphql-codegen style, when this library will never be an executable extension of the JS project.

This I am a bit conflicted about, I do believed one single source of truth for graphql like settings is not a bad idea and would compat fragmentation. Even outside of the JS space there are tools like GraphQL for VSCode, that bring along autocompletion for gql files? Even though pyproject toml like config, does sound delicious :D

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

Also ENV overwrites are a very good idea, maybe focussing on some sensible defaults first (SCHEMA_URL, GENERATED_PATH, etc), what do you think?

Exactly! If you think about it, those "top-level-settings", are the ones that are easy to change via env vars without nesting. The defaults could be perfectly Set in the BaseSettings class definition. Seems like a perfekt match.

It only gets nested, if you enter extensions: turms: ...

I don't know how familiar you are with Docker, but in the end the usage of containerized turms will boil down to a docker-compose.yml which again is a yml file.

Speaking of that, can the docker-compose environment section have a nested structure? Or are this strings only? In the latter case, pydantic Supports parsing env-strings as jsons, which could again come in handy with Tagged Unions for dynamic plugin configurations.

@j-riebe
Copy link
Contributor Author

j-riebe commented Feb 28, 2022

#DontReinventTheWheel
I found this for yaml parsing: Climatecontrol

@jhnnsrs
Copy link
Owner

jhnnsrs commented Mar 14, 2022

Sorry for the late update.
#14 has now BaseSettings replacement for all configuration classes as well as a "test example" how to set them. With the pydantic parsing the setting of nested enviroment variables is possible as well. Do you think climatecontrol brings something else to the mix that would warrant adding it as a dependency? :)

For now I have opted against the nested plugin structure, even though it is conceptually cleaner, so I would be happy to check out your proposal and comments :)

There is now three entrypoints for generation of plugin configuration:

TURMS_$NESTED for configuration applying directly to the generator
TURMS_GRAPHQL for global settings according to the graphql-codegen format
TURMS_PLUGINS_$PLUGINTYPE_$NESTED for configuration on the plugin level

What do you think about this?

@jhnnsrs
Copy link
Owner

jhnnsrs commented Mar 16, 2022

#16
Should now be the complete transition to a more pydantic way of storing configurations

@j-riebe
Copy link
Contributor Author

j-riebe commented Mar 18, 2022

Awesome!

I played around a little bit and overwriting nested values works good.
The only thing that bothers me, is that I can't add or remove plugins entirely via ENV-vars.

To produce a docker image configurable via ENV-vars, we might use a default-configuration that contains all stylers and plugins, etc. and maybe add a flag that disables/enables them.
I'm not quite sure, if the benefit for the containerized usecase is worth an additional parameter for every plugin-config, that is useless for every other usecase ...

Maybe the user just has to write a custom config file and mount it ...

@jhnnsrs What would you suggest?


The suggestion of using ClimateControl is based on a misconception from my side.
I first thought of using it for populating pydantics BaseSettings from .yaml files (instead of .env files), as file-based values have the lowest default-priority in the BaseSettings. They will be overwritten by ENV-vars, which will in turn we be overwritten by explicit kwargs during instantiation.

Because you are passing the config values as init-kwargs, they will always have precedence over ENV-vars (so ENV-vars would be useless to overwrite the config file).
But this holds only for the default settings, which can be changed (see link). BTW, this is the case (!), as the graphql.config.yaml values indeed have a higher priority than ENV-vars right now.

Additionaly, some contributors of ClimateControl played a part in the pydantic implementation for settings nested values in the settings (so this functionality is already part of pydantic).

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

No branches or pull requests

2 participants