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

Update prefect server start command to specify and disable port mappings #2228

Merged
merged 12 commits into from Apr 1, 2020

Conversation

joshmeek
Copy link

@joshmeek joshmeek commented Mar 30, 2020

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • updates docstrings for any new functions or function arguments, including docs/outline.toml for API reference docs (if appropriate)

Note that your PR will not be reviewed unless all three boxes are checked.

What does this PR change?

Open PR up for discussion if this is how we want optional port allocations to work when running prefect server start. This allows ports from the server services to be binded to different ports on the host machine. For this to work I had to add __HOST_PORT options to some of the services in config.toml. I'm guessing this is due to some hardcoded ports that these services use inside their containers. (e.g. overwriting config.server.hasura.port broke communication inside the containers when they ran)

This implementation allows us to set ports through CLI --postgres-port or environment variables PREFECT__SERVER__DATABASE__HOST_PORT however we can definitely go a different route.

This PR also adds the options to disable port mappings on a per-service basis. (e.g. --no-postgres-port)

Closes #2225

Why is this PR important?

Port customization for the start command is a nice convenience to have.

@codecov
Copy link

codecov bot commented Mar 30, 2020

Codecov Report

Merging #2228 into master will increase coverage by 0.32%.
The diff coverage is 100.00%.

Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

I think this makes sense

src/prefect/cli/server.py Outdated Show resolved Hide resolved
src/prefect/cli/docker-compose.yml Outdated Show resolved Hide resolved
@joshmeek joshmeek changed the title Add options to server start CLI command for container host port allocations Update prefect server start command to specify and disable port mappings Mar 31, 2020
@joshmeek joshmeek marked this pull request as ready for review April 1, 2020 13:50
Copy link
Member

@cicdw cicdw left a comment

Choose a reason for hiding this comment

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

👍 only thing I wonder is whether anything wonky could happen when serving a docker setup from a temporary docker compose script location (e.g., if someone leaves the server running for a long time and the temporary docker compose file gets cleaned up by the system, does that matter?)

@joshmeek
Copy link
Author

joshmeek commented Apr 1, 2020

@cicdw I just tested it by spinning it up using the no-port options which creates a tempdir to house the docker compose file. Then while it was running I manually went to the tempdir and deleted the file. Everything continued to run and when I shut it down it all shut down properly 👍

@joshmeek joshmeek merged commit 12d5d58 into master Apr 1, 2020
@joshmeek joshmeek deleted the port_allocation branch April 1, 2020 16:28
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.

Allow setting port for postgres instance in prefect server start
2 participants