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

Changes to support the new RunConfig pattern in core #100

Merged
merged 9 commits into from
Sep 29, 2020
Merged

Conversation

jcrist
Copy link
Contributor

@jcrist jcrist commented Sep 25, 2020

Adds a new flow.run_config field that can be used instead of flow.environment to configure a flow run. There's likely more to do here, but so far this works with PrefectHQ/prefect#3333, see that PR for the full motivation.

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 realize this won't be fully ready until we merge the Core PR, but this looks straightforward to me! I left one comment on backwards compatibility

src/prefect_server/api/flows.py Show resolved Hide resolved
@jcrist
Copy link
Contributor Author

jcrist commented Sep 28, 2020

I'm seeing errors about null run-config values, should that field be nullable (currently it's not, but afaict the server-side default should be being applied)?

@cicdw
Copy link
Member

cicdw commented Sep 28, 2020

@jcrist the server_default is only used if you don't provide a value for run_config when inserting a new row. But our ORM will always provide a value (and in this case it's providing null). If you want to keep the non-nullability, you'll need to put a run_config or {} in the model before the insert.

@codecov-commenter
Copy link

codecov-commenter commented Sep 29, 2020

Codecov Report

Merging #100 into master will increase coverage by 0.16%.
The diff coverage is 100.00%.

@jcrist
Copy link
Contributor Author

jcrist commented Sep 29, 2020

I believe this is good to go.

cicdw
cicdw previously approved these changes Sep 29, 2020
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.

LGTM! Welcome to the server repo @jcrist :D

@jcrist jcrist merged commit 9bd402c into master Sep 29, 2020
@jcrist jcrist deleted the run_config branch September 29, 2020 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants