As a user, I can whitelist packages to sync with standard python syntax #173

werwty · 2018-05-19T01:26:56Z

closes #2041
https://pulp.plan.io/issues/2041

asmacdo

This looks fantastic. This will work well, but I was hoping that the specifier syntax could match a subset of the requirements.txt syntax.

asmacdo · 2018-05-19T02:55:32Z

pulp_python/app/viewsets.py

@@ -121,6 +120,17 @@ class PythonRemoteViewSet(platform.RemoteViewSet):
    serializer_class = python_serializers.PythonRemoteSerializer
    filter_class = PythonRemoteFilter

+    @transaction.atomic


I think we can add the transaction to the serializer code, so we can just use the CreateMixin.create.

asmacdo · 2018-05-19T02:56:21Z

pulp_python/app/serializers.py

+
+        projects = validated_data.pop('projects')
+
+        PythonRemote = python_models.PythonRemote.objects.create(**validated_data)


s/PythonRemote/python_remote/

asmacdo · 2018-05-19T03:28:02Z

pulp_python/app/tasks/sync.py

-            for package in packages:
-                remote_units.append(parse_metadata(metadata['info'], version, package))
-
+            specifier = specifiers.SpecifierSet(project.version)


Optional name suggestion: s/specifier/project_specifier/

asmacdo · 2018-05-19T03:37:55Z

pulp_python/app/tasks/sync.py

-                remote_units.append(parse_metadata(metadata['info'], version, package))
-
+            specifier = specifiers.SpecifierSet(project.version)
+            if specifier.contains(version):


This is a little outside of the scope of _find_remote. What do you think about moving this into _find_delta?

asmacdo · 2018-05-19T03:38:39Z

pulp_python/app/serializers.py

+    def create(self, validated_data):
+        """
+        Creates a PythonRemote
+        Overriding default create() to write the projects nested field


asmacdo · 2018-05-19T03:55:27Z

pulp_python/app/serializers.py

+        allow_blank=True
+    )
+
+    sha256 = serializers.CharField(


requirements.txt accepts hashes in the form sha256:486ea46224d1bb4fb680f34f7c9ad96a8f24ec88be73ea8e5a6c65260e9cb8a7

Often, more than one hash is specified (one for sdist, one for wheel, for example). Could we call this digests and accept a list of strings in the format that is in requirements.txt? This would allow us to support md5 also.

asmacdo · 2018-05-19T03:56:50Z

pulp_python/app/serializers.py

+    )
+
+    sha256 = serializers.CharField(
+        help_text=_("hash"),


Could you add some more here?

asmacdo · 2018-05-19T03:57:18Z

pulp_python/app/tasks/sync.py

+            specifier = specifiers.SpecifierSet(project.version)
+            if specifier.contains(version):
+                for package in packages:
+                    if bool(not project.sha256) ^ bool(package['digests']['sha256'] == project.sha256):


bitwise is cool!

asmacdo · 2018-05-19T04:00:45Z

setup.py

@@ -4,7 +4,8 @@

 requirements = [
    'pulpcore-plugin',
-    'pkginfo'
+    'pkginfo',
+    'packaging'


Optional: it might be good to add a , so future diffs will only be 1 line

asmacdo · 2018-05-19T04:07:09Z

pulp_python/app/serializers.py

+        help_text=_("hash"),
+        required=False,
+    )
+


If we add a create function, we could accept strings in the requirements.txt syntax instead a dictionary.

I think a dictionary is the right way to go for this endpoint. I don't want to make requirements.txt the default syntax for creating project specifiers (since we do want Pipfile and Pipfile.lock support.

We need to discuss how to implement these different ways to pass different files; either through one endpoint for each of them, or through the CLI side.
Whichever way we do decide, I think this endpoint should be left generic

dralley · 2018-05-19T04:25:24Z

Tags por favor :)

dralley · 2018-05-22T01:07:27Z

This looks great @werwty , I'll test it tomorrow

werwty · 2018-05-22T01:41:41Z

@dralley hold off for a day please, I still need to refactor sync and update it based on asmacdo's other comments.

pep8speaks · 2018-05-25T20:44:43Z

Hello @werwty! Thanks for updating the PR.

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on June 01, 2018 at 18:08 Hours UTC

asmacdo · 2018-05-29T14:11:56Z

As much as I'd like to merge, I think we really have to wait until https://pulp.plan.io/issues/3705 is fixed.

dralley · 2018-05-31T19:39:39Z

pulp_python/app/serializers.py

+        help_text=_("A python project name.")
+    )
+
+    version_specifier = serializers.CharField(


It would be good to add validation to the version specifiers, although it may be non-trivial enough to make a future task.

http://www.django-rest-framework.org/api-guide/serializers/#field-level-validation

dralley · 2018-05-31T19:44:25Z

pulp_python/app/models.py

+        version==1.0.0 will sync all distributions matching version
+        version~=1.0.0 will sync all major version 1 distributions
+        version==1.0.0 digests=["sha256:0000"] will only sync the distributions matching the hash
+        name=projectname without specifiers will sync every distribution in the project.


Does digests work separately from a version specifier? If yes, could you provide an example for just digests?

Could you provide another example using >=0.9, <1.0?

@dralley Ah I thought you meant add an example to the workflow docs. I'll add those to the docstring too

dralley · 2018-05-31T20:00:29Z

pulp_python/app/serializers.py

@@ -22,6 +23,50 @@ class Meta:
        fields = ('name',)


+class DistributionDigestSerializer(serializers.ModelSerializer):


Same here, we should validate that the digest type is supported and that the digest is the correct length for whatever type it is.

I'm not sure about this, we really "support" any digest type. During sync time all we do is check that the type and value in the JSON API's digest field is the same.

Good point, never mind then.

asmacdo · 2018-05-31T19:30:29Z

docs/workflows/sync.rst

-        url='https://repos.fedorapeople.org/repos/pulp/pulp/fixtures/python-pypi/' \
-        projects='["shelf-reader"]'
+        url='https://pypi.org/' \
+        projects:='[{"name": "django", "version_specifier":"~=2.0",


do we need backslashes on the other lines as well?

nope, it's already enclosed in a ' so bash knows to wait for it to close

asmacdo · 2018-05-31T19:32:44Z

pulp_python/app/models.py

+
+class ProjectSpecifier(Model):
+    """
+    A specifier of a python project dependency.


s/dependency//

asmacdo · 2018-05-31T19:35:07Z

pulp_python/app/models.py

+
+    Example:
+
+        version==1.0.0 will sync all distributions matching version


This should use the same data structure that is passed to it, version==1.0.0 isn't actually a valid value.

asmacdo · 2018-05-31T19:36:11Z

pulp_python/app/models.py

+    Fields:
+
+        project_name (models.TextField): The name of a python project
+        version (modles.TextField): A filter for the versions to sync


version_specifier: Used to filter the versions of a project to sync

also typo: s/modles/models

asmacdo · 2018-05-31T19:39:55Z

pulp_python/app/serializers.py

+    """
+    A serializer for the Distribution Digest in a Project Specifier
+    """
+    type = serializers.CharField(


i thought we settled on this being combined with digest like "sha256:3d991651559..."

I think not combined is nicer, and we don't need to split when comparing hashes

cool. how about a choice field then? Re: https://github.com/pulp/pulp_python/pull/173/files#r192219408, I think we probably do want to only support certain digests. For the moment, we only use digests to filter the content that we download, but it would be good form to check that hash after we download it.

This digest is user provided and only used to filter. The hash checking logic would not belong on this but rather on the digest of the created artifact

asmacdo · 2018-05-31T19:56:19Z

pulp_python/app/serializers.py

+
+        # Remove all project specifier related by foreign key to the remote if it is not a
+        # partial update or if new projects list has been passed
+        if not partial or projects:


I expect PATCH to not delete unmentioned fields at the top level only, so IMO, a PATCH that gives a list of project specifiers should not be additive.

asmacdo · 2018-05-31T19:58:57Z

pulp_python/app/tasks/sync.py

@@ -75,27 +78,42 @@ def _fetch_inventory(version):
    return inventory


-def _fetch_remote(remote):
+def _fetch_specified_remotes(remote, project_specifiers):


how about _fetch_remote_metadata

(good plan)

But this only fetches some remote metadata, maybe fetch_specified_remote_metadata? (naming is hard)

That works for me. or fetch_sepecified_metadata. Its sort of obvious that is going to come from the remote.

asmacdo · 2018-05-31T20:05:19Z

pulp_python/app/models.py

+                                          on_delete=models.CASCADE)
+
+
+class ProjectSpecifier(Model):


This is actually a "PackageSpecifier"

We've been using the project/version/distribution names throughout. Why change this to package now?

Hmm. DistributionSpecifier could be confused with our Distribution, and PackageSpecifier is clear enough. The point is it doesn't specify projects, even though that is how they are organized. They actually specify a set of Python Distributions.

asmacdo · 2018-05-31T20:20:44Z

pulp_python/app/tasks/sync.py

+            if specifier.contains(version):
+                for package in packages:
+                    # add the package if the project specifier does not have an associated digest
+                    if digests.count() == 0:


doesnt if digests work?

nope, since digests is a QuerySet that doesn't evaluate to false

I can just use exists()

asmacdo · 2018-05-31T20:22:09Z

pulp_python/app/tasks/sync.py

+                    # otherwise check each digest to see if it matches the specifier
+                    else:
+                        for type, digest in package['digests'].items():
+                            digest_match = digests.filter(type=type, digest=digest).count()


I think we want:
if digests.filter(type=type, digest=digest).exists()

asmacdo · 2018-05-31T20:24:45Z

docs/workflows/sync.rst

@@ -35,8 +35,10 @@ You can use any Python remote to sync content into any repository::

    $ http POST $BASE_ADDR/pulp/api/v3/remotes/python/ \
        name='bar' \
-        url='https://repos.fedorapeople.org/repos/pulp/pulp/fixtures/python-pypi/' \
-        projects='["shelf-reader"]'
+        url='https://pypi.org/' \


I'm not sure this is actually necessary, or would even work. The JSON spec doesn't allow a trailing comma IIRC.

edit: unless this has been fixed already, without hiding the comment since it was on the wrong line. I'm just assuming you meant trailing comma based on what it is currently.

no i just saw the other comma and assumed this needed one as well.

dralley · 2018-05-31T20:37:37Z

pulp_python/app/serializers.py

@@ -141,10 +186,7 @@ class PythonPackageContentSerializer(core_serializers.ContentSerializer):
    def create(self, validated_data):
        """
        Creates a PythonPackageContent


newline + args

dralley · 2018-05-31T21:15:52Z

Make sure all the smash tests pass -- it may require modifying some of them.

Rebase if you haven't already as I fixed an issue causing some smash tests to fail earlier.

dralley · 2018-05-31T22:22:23Z

.travis/install.sh

@@ -37,5 +37,5 @@ else
  popd
 fi

-cd pulp_python


This should be incorrect, after entering the directories above it does a popd, so it should be ending up where it started.

Is that not happening properly?

It is not: https://travis-ci.org/pulp/pulp_python/jobs/386389531

ohhh I see this is after we renamed the branch so the branch checkout errors out and it doesn't popd

@werwty this is why

https://travis-ci.org/pulp/pulp_python/jobs/386389531#L550

The branch 3.0-dev no longer exists, therefore the git checkout errors, therefore bash's && expression falls through early.

edit: page wasn't refreshed, you got to it first xD

@werwty it does still need the cd pulp_python line though

Also could you edit the install docs?

dralley · 2018-05-31T22:44:08Z

.travis/install.sh

@@ -13,7 +13,7 @@ pip install -r test_requirements.txt
 cd .. && git clone https://github.com/pulp/pulp.git

 if [ -z $PULP_PR_NUMBER ]; then
-  pushd pulp && git checkout 3.0-dev && popd
+  pushd pulp && git checkout master && popd


Probably no need to have this branch anymore since it defaults to master. Swap -z with -n (I think, I had to reference the docs and haven't tested)

The source install docs need to be changed to remove the "checkout 3.0-dev" bit

asmacdo

A few nits, but no need to re-review. Great work @werwty, this feature is going to be awesome.

asmacdo · 2018-06-01T17:40:39Z

pulp_python/app/models.py

+
+    Example:
+
+        version_specifier==1.0.0 will sync all distributions matching version


I still think thes examples could be misleading.

Also, s/sync/match/ throughout this docstring

In what way are these examples misleading? (How can I clarify them?)

version_specifier==1.0.0 is not a valid version_specifier
I think even just adding punctuation will clarify

"version_specifier: ==1.0.0 will match all distributions ..."

asmacdo · 2018-06-01T17:41:11Z

pulp_python/app/models.py

+
+    Fields:
+
+        project_name (models.TextField): The name of a python project


s/project_name/name/

asmacdo · 2018-06-01T17:42:06Z

pulp_python/app/serializers.py

 from rest_framework import serializers
 from rest_framework_nested.relations import NestedHyperlinkedRelatedField
+


no space here. pulpcore is a 3rd party library from the plugin's perspective.

asmacdo · 2018-06-01T17:43:38Z

pulp_python/app/serializers.py

+
+    def validate_version_specifier(self, value):
+        """
+        Check that the Version Specifier valid


s/valid/is valid/

Nice!

asmacdo · 2018-06-01T17:47:41Z

pulp_python/app/tasks/sync.py

+
+                specifier = specifiers.SpecifierSet(project.version_specifier)
+
+                # Note: SpecifierSet("").contains(version) will return true for released versions


Could you add this to the help_text on the serializer?

closes #2041 https://pulp.plan.io/issues/2041

asmacdo suggested changes May 19, 2018

View reviewed changes

werwty added feature request New feature request 3.0 labels May 19, 2018

werwty force-pushed the projspecifier branch from 90ddb7e to 617380a Compare May 19, 2018 13:48

werwty added the Work In Progress label May 22, 2018

werwty force-pushed the projspecifier branch from 617380a to 31299e4 Compare May 25, 2018 20:44

werwty force-pushed the projspecifier branch from 31299e4 to c0111a3 Compare May 25, 2018 20:48

werwty removed the Work In Progress label May 25, 2018

werwty force-pushed the projspecifier branch 3 times, most recently from 5462e80 to 506dfc2 Compare May 31, 2018 19:36

dralley reviewed May 31, 2018

View reviewed changes

asmacdo suggested changes May 31, 2018

View reviewed changes

asmacdo reviewed May 31, 2018

View reviewed changes

werwty force-pushed the projspecifier branch from 506dfc2 to 89b49d6 Compare May 31, 2018 20:27

dralley reviewed May 31, 2018

View reviewed changes

werwty force-pushed the projspecifier branch 6 times, most recently from c39f08a to d4d8da9 Compare May 31, 2018 21:58

dralley reviewed May 31, 2018

View reviewed changes

werwty force-pushed the projspecifier branch 2 times, most recently from bcc8eed to 08886b4 Compare May 31, 2018 22:42

dralley reviewed May 31, 2018

View reviewed changes

werwty force-pushed the projspecifier branch 8 times, most recently from 3925700 to 07f7663 Compare June 1, 2018 17:06

asmacdo approved these changes Jun 1, 2018

View reviewed changes

werwty force-pushed the projspecifier branch 2 times, most recently from c4460b1 to 8452656 Compare June 1, 2018 18:06

As a user, I can whitelist packages to sync with standard python syntax

fecf631

closes #2041 https://pulp.plan.io/issues/2041

werwty force-pushed the projspecifier branch from 8452656 to fecf631 Compare June 1, 2018 18:08

werwty merged commit 00af7fa into pulp:3.0-dev Jun 1, 2018


		projects = validated_data.pop('projects')

		PythonRemote = python_models.PythonRemote.objects.create(**validated_data)

		@@ -22,6 +23,50 @@ class Meta:
		fields = ('name',)


		class DistributionDigestSerializer(serializers.ModelSerializer):


		Example:

		version==1.0.0 will sync all distributions matching version


		Example:

		version_specifier==1.0.0 will sync all distributions matching version


		Fields:

		project_name (models.TextField): The name of a python project

		from rest_framework import serializers
		from rest_framework_nested.relations import NestedHyperlinkedRelatedField


		specifier = specifiers.SpecifierSet(project.version_specifier)

		# Note: SpecifierSet("").contains(version) will return true for released versions

As a user, I can whitelist packages to sync with standard python syntax #173

As a user, I can whitelist packages to sync with standard python syntax #173

Conversation

werwty commented May 19, 2018

asmacdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented May 19, 2018

dralley commented May 22, 2018

werwty commented May 22, 2018

pep8speaks commented May 25, 2018 • edited Loading

Comment last updated on June 01, 2018 at 18:08 Hours UTC

asmacdo commented May 29, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley commented May 31, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley May 31, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dralley May 31, 2018 • edited Loading

Choose a reason for hiding this comment

asmacdo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pep8speaks commented May 25, 2018 •

edited

Loading

dralley May 31, 2018 •

edited

Loading

dralley May 31, 2018 •

edited

Loading

dralley May 31, 2018 •

edited

Loading