Navigation Menu

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

Fix some interpolation of config in Prefect server dev commands #2299

Merged
merged 4 commits into from Apr 8, 2020

Conversation

lauralorenz
Copy link

@lauralorenz lauralorenz commented Apr 8, 2020

  • Convert ui_path when read from environment variable into a Path object.
  • Use config file's host and port for hasura and apollo when starting services with dev services.

Thanks for contributing to Prefect!

Please describe your work and make sure your PR:

  • [n/a] adds new tests (if appropriate)
  • updates CHANGELOG.md (if appropriate)
  • [n/a] 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?

Fixes some config interpolation that wasn't happening when running services independently/not with docker.

Why is this PR important?

Needed to dev on Apollo from Prefect Core; Apollo services could not build Apollo schema or healthcheck Apollo when running from dev services without it. Also needed to be able to build the UI from the env variable path instead of the default.

- Convert ui_path when read from environment variable into a Path object.
- Use config file's host and port for hasura and apollo when starting services with `dev services`.
@jlowin
Copy link
Member

jlowin commented Apr 8, 2020

@lauralorenz makes total sense for dev work but I think these env vars set the docker-compose environment as well, and I think that localhost won't properly be recognized inside the docker compose environment; it'll need to resolve the service name ("hasura", "graphql", etc.)

Potentially, since we know the docker-compose file with certainty, we should just not all the hostname to be specified and use hardcode the service names instead in docker-compose.yaml

@lauralorenz
Copy link
Author

@jlowin

I have that the prefect server start command aka the docker compose based setup is controlled by config set in this file: https://github.com/PrefectHQ/prefect/blob/master/src/prefect/cli/server.py#L26-L39

and that the changes in this PR are to a different file, which is only used by the prefect-server dev services and prefect-server infrastructure commands.

…ices-config-interpolation

# Conflicts:
#	CHANGELOG.md
@jlowin
Copy link
Member

jlowin commented Apr 8, 2020

Ah, I apologize, I didn't realize it was two different files! 👍

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.

To avoid the reasonable confusion that @jlowin had I think we should rename the server/cli/dev make_env function to make_dev_env to clarify that it is purely for development purposes. (and it's associated calls)

@codecov
Copy link

codecov bot commented Apr 8, 2020

Codecov Report

Merging #2299 into master will increase coverage by 0.01%.
The diff coverage is n/a.

@cicdw cicdw merged commit 3bc254d into master Apr 8, 2020
@cicdw cicdw deleted the fix-server-dev-services-config-interpolation branch April 8, 2020 19:23
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.

None yet

4 participants