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

Add Task IDs and remove related cruft #416

Merged
merged 12 commits into from Dec 19, 2018
Merged

Add Task IDs and remove related cruft #416

merged 12 commits into from Dec 19, 2018

Conversation

jlowin
Copy link
Member

@jlowin jlowin commented Dec 19, 2018

Closes #383

This PR primarily introduces a UUID for Task objects.

Secondarily, it removes crutches that were introduced in the past to work around the fact tasks didn't have IDs.

Background

Giving a Task an ID is trickier than it sounds, because any given Task object can be reused in multiple Flows. This can lead to a situation where users try to deploy two or more flows that contain tasks with the same ID.

However, as development has progressed, we've found that this situation is unlikely; almost all task-based operations create copies of the parent task, and copies can be given new IDs. Furthermore, since it is unexpected that anyone would hardcode an ID dependency (preferring instead to access it at runtime), it would be fine to simply inform users that they had an (unlikely) ID conflict and invite them to globally randomize all IDs.

This all goes without saying, of course, that UUIDs are only probabilistically unique to begin with, and collisions are always possible (but not really).

Since we don't have IDs, we introduced a few helpers to work around it -- like assigning random IDs in a task_info dict on the Flow itself, and passing IDs around in a Marshmallow context during deserialization. As time has gone on, those helpers have grown like weeds; task_info in particular came to represent everything from the task's ID to its mapped status.

Contributions

Primarily, this PR adds an id field for tasks, similar to the one on flows.

In addition, it cleans up some of the messy context-based solutions to serializing/deserializing task objects with IDs. One vestigial but useful remnant is that when tasks are deserialized, they put themselves in a task cache that can be reused by any other serializers. This allows, for example, multiple Edges to all deserialize the same Task object even if all they have is its string ID.

Lastly, it completely removes the task_info dictionary.

@jlowin jlowin changed the title Update ids Add Task IDs and remove related cruft Dec 19, 2018
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.

Just a question that I'm 99% sure I already know the answer to; otherwise, nice!

- value (str): a UUID-formatted string
"""
try:
uuid.UUID(value)
Copy link
Member

Choose a reason for hiding this comment

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

I assume this is just a formatting check, and not intended to be saved?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, felt like the quickest way to ensure it was a valid UUID. The class constructor will raise an error if it isn't.

@jlowin jlowin merged commit 1ca8c99 into master Dec 19, 2018
@jlowin jlowin deleted the update-ids branch December 19, 2018 17:30
jlowin added a commit that referenced this pull request Dec 19, 2018
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

3 participants