-
Notifications
You must be signed in to change notification settings - Fork 167
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 katello_content_view_filter module #153
Conversation
7292119
to
3f9abc2
Compare
7164556
to
186c839
Compare
Depending PRs merged and tests passed 🎆 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not a full review, just one thing that did not hide properly.
@@ -229,12 +233,12 @@ def update_fields(new, old, fields): | |||
|
|||
|
|||
# Common functionality to manipulate entities | |||
def naildown_entity_state(entity_class, entity_dict, entity, state, module): | |||
changed, _ = naildown_entity(entity_class, entity_dict, entity, state, module) | |||
def naildown_entity_state(entity_class, entity_dict, entity, state, module, check_missing=[]): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mutable objects as default Argumnents are almost never, what you expect. Please use None
here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did it as it saves me having to check if it's iterable later, anyway updated.
Thanks @mdellweg |
Requires:
SatelliteQE/nailgun#504SatelliteQE/nailgun#505SatelliteQE/nailgun#506SatelliteQE/nailgun#508Katello/katello#7560Katello/katello#7492I wasn't going to raise this until the above PRs were merged, but it seems the Nailgun ones, in particular, are taking a while. Anyone interested in testing this is welcome to use https://github.com/sean797/nailgun/tree/mix which has the 4 commits cherry-picked.