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 idempotency key to flows.create_flow #116

Merged
merged 16 commits into from
Oct 28, 2020
Merged

Add idempotency key to flows.create_flow #116

merged 16 commits into from
Oct 28, 2020

Conversation

zanieb
Copy link
Contributor

@zanieb zanieb commented Oct 27, 2020

Summary

Adds optional idempotency keys to the create_flow API, the most recent key for a flow group is stored in the settings dict and is referenced when creating a flow to prevent sequential flow creation with the same key.

Importance

  • Users register flows repeatedly with automation and needlessly bump versions

Future work

This only works for sequential flow creation and we may want support in the future to prevent re-registrations for old flow versions. However, this would involve adding a column to the Flow table and additional complexity. We also don't really have support for old versions right now so it isn't quite necessary.

Checklist

This PR:

  • adds new tests (if appropriate)
  • adds a change file in the changes/ directory (if appropriate)

Naive first implementation
@zanieb zanieb changed the title WIP: Add idempotency_key to create_flow_run Add idempotency_key to create_flow_run Oct 27, 2020
@zanieb zanieb changed the title Add idempotency_key to create_flow_run Add idempotency_key to create_flow Oct 27, 2020
@codecov-io
Copy link

codecov-io commented Oct 27, 2020

Codecov Report

Merging #116 into master will increase coverage by 0.04%.
The diff coverage is 100.00%.

@zanieb zanieb marked this pull request as ready for review October 27, 2020 18:21
@zanieb zanieb requested review from cicdw and jlowin as code owners October 27, 2020 18:21
@zanieb zanieb changed the title Add idempotency_key to create_flow Add idempotency key to flows.create_flow Oct 27, 2020
@zanieb zanieb self-assigned this Oct 27, 2020
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
src/prefect_server/api/flows.py Show resolved Hide resolved
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
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.

A few very superficial comments

src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
src/prefect_server/api/flows.py Outdated Show resolved Hide resolved
zanieb and others added 3 commits October 28, 2020 09:00
Co-authored-by: Chris White <[email protected]>
And fix accidental override of flow local in that case
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.

Could you add one final test? I want to ensure we always update the idempotency key so that the following situation works as expected:

  • flow.register("Blah", idempotency_key="foo") # v1
  • flow.register("Blah") # v2
  • flow.register("Blah", idempotency_key="foo") <-- should actually register a new version instead of using v2

So a test that creates a flow w/ key, creates a flow without, then creates a final flow with the original key should register a new version.

@zanieb zanieb requested a review from cicdw October 28, 2020 16:38
Including X -> Y -> X and X -> None -> X
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

pytest.param(("foo", None, "foo"), id="same key with empty between"),
pytest.param(("foo", "bar", "foo"), id="same key with different between"),
],
)
Copy link
Member

Choose a reason for hiding this comment

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

nice, well done

@cicdw cicdw merged commit 69e0414 into master Oct 28, 2020
@cicdw cicdw deleted the idempotency-register branch October 28, 2020 17:09
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