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 prefect server config command to output a compose file #4176

Merged
merged 17 commits into from
Mar 3, 2021

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Feb 25, 2021

Summary

Instead of starting the server, provide the configured file for a user to spin it up themselves.

prefect server config writes a configure docker-compose yaml to standard output using docker-compose config to resolve environment variables. It takes all the same (non pull/start) options as prefect server start

Changes

  • Adds config to prefect server CLI
  • Refactors docker-compose file and environment generation
  • Warns on collisions between the current environment and CLI commands

Importance

Closes #4151

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Instead of starting the server, provide the configured files for a user

- Removes the fname write from `make_env`, looks unsused
@sm-Fifteen
Copy link

prefect agent local has a similar feature, local install, which takes (most of) the same parameters as local start but generates a supervisord.conf file. I would argue that making --dump a subcommand instead of an option would be better, as far as CLI uniformity is concerned, though being able to specify a target directory is a nice plus.

Aside from that, this all looks great.

@zanieb
Copy link
Contributor Author

zanieb commented Feb 25, 2021

Yeah I could be convinced to do a sub-command instead. I just didn't want to replicate all the code in prefect server start or refactor it all to be in a separate function. Thoughts @jcrist ?

@jcrist
Copy link

jcrist commented Feb 25, 2021

I think a separate command makes sense. I'd like to transition prefect agent local install/prefect agent kubernetes install to commands with more specific names (install indicates that it installs something but it actually doesn't, it just outputs a file). Would be good to start with a good name here, perhaps prefect server gen-compose, or something similar?

@sm-Fifteen
Copy link

I think a separate command makes sense. I'd like to transition prefect agent local install/prefect agent kubernetes install to commands with more specific names (install indicates that it installs something but it actually doesn't, it just outputs a file). Would be good to start with a good name here, perhaps prefect server gen-compose, or something similar?

How about prefect server persist-config --label=foo --env=BAR=baz --postgres-port=1234?

I recall some npm tool - I believe it was react-create-app - having an eject subcommand for that exact purpose: to let people switch to an unmanaged configuration, making the user able to alter all the config files themselves to better fit their deployment methods. Not that I'm putting any weight towards this command name in particular, but I figured it was worth pointing out.

@jcrist
Copy link

jcrist commented Feb 25, 2021

persist-config sounds like it's writing a file somewhere. I think this should output text, and can be piped to a file as needed (or just piped into docker-compose directly). Config also doesn't really indicate what the "config" file is, whereas something with compose` in the name better indicates it's a docker-compose setup. No strong thoughts on a name beyond that.

@sm-Fifteen
Copy link

persist-config sounds like it's writing a file somewhere. I think this should output text, and can be piped to a file as needed (or just piped into docker-compose directly). Config also doesn't really indicate what the "config" file is, whereas something with compose` in the name better indicates it's a docker-compose setup. No strong thoughts on a name beyond that.

That could work if this didn't need writing content to two files to work, the generated docker-compose.yml file won't work without a dozen mandatory env variables plucked from Prefect's config.toml, which need to be saved to a separate file for docker-compose to load alongside it.

Though I guess one could always tell people to do something like prefect server gen-compose 1> ./docker-compose.yml 3> ./.env or something, it's not like there's a limit to the amount of output streams a program is allowed to have.

@jcrist
Copy link

jcrist commented Feb 25, 2021

the generated docker-compose.yml file won't work without a dozen mandatory env variables plucked from Prefect's config.toml, which need to be saved to a separate file for docker-compose to load alongside it.

Oh, I hadn't realized that. Why not write the values directly in the generated compose file? I was viewing this as effectively "freezing" the config for later use. A single generated file feels simpler to work with to me than having to move around two files with the proper paths.

@zanieb
Copy link
Contributor Author

zanieb commented Feb 25, 2021

I think it makes sense to inject the variables into the file as well. I'm honestly quite confused at the way the docker-compose is written right now where there are ${VAR-default} everywhere but the defaults are actually determined by the click CLI option defaults. (I think a bunch of this was copied over from the Server dev code and has just grown over time)

I've started on refactoring this a bit further to separate it into another command and it's kind of just a mess. I can just rehaul it.

@jcrist
Copy link

jcrist commented Feb 25, 2021

Yeah, I think that might just be historical and could all be removed/redone. Feel free to change the compose setup however you want to make things simpler.

Refactors common options and simplifies env creation
@zanieb
Copy link
Contributor Author

zanieb commented Feb 26, 2021

It turns out there's a docker-compose command for this! docker-compose config so I've named this to match: prefect server config

This means that I do not have to touch the docker-compose file. We may want to do this eventually/later but this makes this change less likely to break things for users.

@zanieb zanieb changed the title Add the option to dump a docker-compse file and env Add prefect server config command to output a compose file Feb 26, 2021
@zanieb zanieb marked this pull request as ready for review March 2, 2021 20:02
@zanieb zanieb requested a review from jcrist as a code owner March 2, 2021 20:02
@zanieb
Copy link
Contributor Author

zanieb commented Mar 2, 2021

Still needs a test for prefect server config -- I'm not sure how I want to test it yet. Any tests other than "this command runs" would basically replicate prefect server start tests unless we want to actually call "docker-compose config". I'm not sure it's unreasonable to do the same thing as the server start setup/teardown test to ensure variables are passed correctly

Copy link

@jcrist jcrist left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One nit, otherwise this looks good to me.

Can you also add the config command to the CLI docs here:

commands = ["start", "create_tenant"]
. Looks like prefect server stop needs to be added as well.

src/prefect/utilities/compatibility.py Outdated Show resolved Hide resolved
@jcrist
Copy link

jcrist commented Mar 3, 2021

I'm not sure it's unreasonable to do the same thing as the server start setup/teardown test to ensure variables are passed correctly

I think doing a test with no flags (things run fine by default) and one with lots of flags/env-vars (test args get forwarded properly) would be sufficient. By inspection the same helpers are used, so if the actual env/flag handling logic is tested elsewhere already we only need to ensure things are hooked together properly.

@zanieb
Copy link
Contributor Author

zanieb commented Mar 3, 2021

Docs look good: https://deploy-preview-4176--prefect-docs.netlify.app/api/latest/cli/server.html#config

Messed up a test... should be an easy fix.

@zanieb zanieb requested a review from jcrist March 3, 2021 17:56
@jcrist jcrist merged commit f9dc7c4 into master Mar 3, 2021
@jcrist jcrist deleted the server-start-dump branch March 3, 2021 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants