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

Allow content to be copied by href #1602

Merged
merged 1 commit into from Feb 20, 2020

Conversation

daviddavis
Copy link
Contributor

@daviddavis daviddavis commented Feb 19, 2020

@pep8speaks
Copy link

pep8speaks commented Feb 19, 2020

Hello @daviddavis! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-02-20 17:37:02 UTC

@@ -614,6 +614,11 @@ class CopySerializer(serializers.Serializer):
required=False,
)

content = serializers.ListField(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Open to suggestions about this parameter name, etc.

@@ -78,22 +89,11 @@ def _do_test(self, criteria, expected_results):
get_added_content_summary(dest_repo), expected_results,
)

def test_copy_none(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed the ability to copy nothing. Not sure there was a use case for it?

if 'criteria' in data and 'content' in data:
raise serializers.ValidationError(
_("Criteria and content fields cannot both be set.")
)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm debating about dropping this? I think Katello will want to specify hrefs and criteria.

@dralley dralley force-pushed the copy-api-pulp3 branch 2 times, most recently from 8d236c5 to 82a9ccc Compare February 20, 2020 14:35
@daviddavis daviddavis force-pushed the content-ids branch 2 times, most recently from 48e90da to 3bd7515 Compare February 20, 2020 17:03
@daviddavis daviddavis marked this pull request as ready for review February 20, 2020 17:03
@daviddavis daviddavis requested a review from a team February 20, 2020 17:03
@dralley
Copy link
Contributor

dralley commented Feb 20, 2020

Could you leave a link to the issue in your commit, and also add to the issue an API usage example that documents how it works?

@daviddavis
Copy link
Contributor Author

@dalley done. let me know if you have any other feedback.

@dralley
Copy link
Contributor

dralley commented Feb 20, 2020

LGTM. I will be out tomorrow so feel free to merge.

@daviddavis daviddavis merged commit 2fe71dd into pulp:copy-api-pulp3 Feb 20, 2020
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