Skip to content
This repository has been archived by the owner on Dec 7, 2022. It is now read-only.
/ pulp Public archive

As a user, I can rest easy in the knowledge that Pulp's Celery tasks are not serialized dangerously #2241

Merged
merged 3 commits into from
Dec 15, 2015

Conversation

ipanova
Copy link
Member

@ipanova ipanova commented Dec 11, 2015

@bmbouter
Copy link
Member

I think the code is not Python 2.6 compliant. Can you update the PR so that Jenkins is happy.

@bmbouter
Copy link
Member

I'm glad the commits are preserved. Any changes to this PR can you squash into your commit, and preserve the other 2 (as you've already done).

@bmbouter
Copy link
Member

Please add details to the commit message outlining the changes. I think the topics are:

  1. the inheritance changes to introduce PulpTask
  2. the serialization/deserialization layer which handles ObjectIds
  3. The UnitAssociation issues to_json() from_json()
  4. The changes to criteria

"""
Convert a dictionary representation of the UnitAssociationCriteria into a new
UnitAssociationCriteria object.
The output of to_dict() is suitable as input to this method.
Copy link
Member

Choose a reason for hiding this comment

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

Consider adding a newline above this to separate it from the paragraph above it.

@bmbouter bmbouter self-assigned this Dec 11, 2015


@celery.task(base=PulpTask)
def publish(repo_id, distributor_id, overrides=None):
Copy link
Member

Choose a reason for hiding this comment

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

Was this moved from somewhere else? I can't think of why this is added.

@bmbouter
Copy link
Member

@ipanova This looks really great, I left a bunch of small comments to handle. The two things I don't understand and I believe should be removed are these comments #2241 (comment) and #2241 (comment)

Once all of that gets fixed I think it will be good for merging. I can review again on Monday if you want or not, or you can merge it otherwise.

@bmbouter bmbouter assigned ipanova and unassigned bmbouter Dec 11, 2015
@ipanova ipanova closed this Dec 14, 2015
@ipanova ipanova reopened this Dec 14, 2015
@bmbouter
Copy link
Member

@ipanova The most recent push addresses the technical concerns. Can you rebase against the latest master so it can be cleanly merged?

Shubham Bhawsinka and others added 3 commits December 15, 2015 10:43
* Recursive serialization and deserialization
* Force all Pulp Celery tasks to inherit from PulpTask
closes pulp#23
https://pulp.plan.io/issues/23

UnitAssociationCriteria arrives as dict in the task code, when it needs to be
UnitAssociationCriteria object. to_dict() method was introduced to serialize
UnitAssociationCriteria to a dictionary prior to it being sent as an argument
to apply_sync(). the from_dict() method was introduced to return UnitAssociationCriteria
object from a dictionary that was produced by to_dict()

This commit also introduces a testcase that checks that all Celery tasks defined in Pulp
inhherit from PulpTask and not from Celery directly.

There have been done also changes to criteria. Due to the introduction of de/sereliazions
methods, criteria's value cannot be None. Code and tests were updated accordigly.
@ipanova
Copy link
Member Author

ipanova commented Dec 15, 2015

@bmbouter conflicts resolved, jenkins is happy :)

@bmbouter
Copy link
Member

@ipanova great job! Also thanks to @shubham90 for his work on this too.

@ipanova ipanova merged commit ed36708 into pulp:master Dec 15, 2015
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants