-
Notifications
You must be signed in to change notification settings - Fork 77
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 sync_mode parameter to endpoint that deploys sync task #159
Conversation
Just noticed #153 |
Use tags : P |
Looks like the revised changeset will have a sync_mode implementation. I will revise this PR to use that once it is merged, for now leaving as WIP |
pulp_python/app/viewsets.py
Outdated
# Get sync_mode, default to additive | ||
sync_mode = request.data.get('sync_mode', 'additive').lower() | ||
|
||
if sync_mode not in ['mirror', 'additive']: |
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.
We should add a ChoiceField to the serializer, which will handle the validation for us. I already added choices as a constant.
https://github.com/pulp/pulp/blob/3.0-dev/common/pulpcore/common/constants.py#L30-L37
6739b0e
to
cff7966
Compare
pulp_python/app/viewsets.py
Outdated
@@ -156,13 +156,15 @@ def sync(self, request, pk): | |||
) | |||
serializer.is_valid(raise_exception=True) | |||
repository = serializer.validated_data.get('repository') | |||
mirror = serializer.validated_data.get('mirror', True) |
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.
This value defaults to True
in core, and that serializer is how this option is documented. So I think we shouldn't default here, since our default wouldn't be connected to the schema.
https://github.com/pulp/pulp/blob/cdf6133394d99bbc21a69fae4599d8271d6b344c/pulpcore/pulpcore/app/serializers/repository.py#L142
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.
Requested change from @asmacdo was made, looks good now.
Thanks @CodeHeeler |
closes #3493
https://pulp.plan.io/issues/3493