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

Add an endpoint to return a CollectionVersion's resolved dependencies #522

Closed

Conversation

bmclaughlin
Copy link
Contributor

Issue: AAH-8

- Notes conflicts in returned dictionary
Issue: AAH-8
@bmbouter
Copy link

Generally the entire Galaxy API V3 is put into pulp_ansible and then subclassed back to galaxy_ng. Have we considered where the right place to contribute this is?

@@ -17,6 +17,8 @@
CollectionVersionListSerializer as _CollectionVersionListSerializer,
)

from pulp_ansible.app.models import CollectionVersion
Copy link
Contributor

Choose a reason for hiding this comment

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

PEP8:

Imports should be grouped in the following order:

Standard library imports.
Related third party imports.
Local application/library specific imports.

from django.http import HttpResponseRedirect, StreamingHttpResponse
from django.shortcuts import get_object_or_404
from django.urls import reverse

from functools import reduce
from operator import __and__ as AND
Copy link
Contributor

Choose a reason for hiding this comment

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

Please use operator.and_

https://docs.python.org/3.9/library/operator.html

For backward compatibility, many of these have a variant with the double underscores kept. The variants without the double underscores are preferred for clarity.

offset = 1
operand = d[:offset]
version = d[offset:]
if '>=' == operand:
Copy link
Contributor

Choose a reason for hiding this comment

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

You can use the "dispatch dictionary" pattern to replace this if-elif-else construction.

if resolved_version_number:
resolved_deps[key] = resolved_version_number
else:
resolved_deps[key] = f"Conflict! {deps}"
Copy link
Contributor

Choose a reason for hiding this comment

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

In case of dependency conflict it does not produce an error, but returns formatted string. Could you please give examples of API response format?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

With conflict:

{
    "bmclaughlin.conflict2": "0.0.3",
    "bmclaughlin.deps2": "Conflict! {'bmclaughlin.deps2': '>0.1.0,<0.1.0'}"
}

Without conflict:

{
    "bmclaughlin.deps2": "0.0.3",
    "bmclaughlin.deps1": "0.0.4",
    "bmclaughlin.execution_env_test": "0.1.2",
    "ansible.netcommon": "1.3.0"
}

Copy link
Contributor

Choose a reason for hiding this comment

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

Response format in case of conflict is vague and requires parsing on a client.

Can you please give a link to this API format specification?

- operator.and_ usage
- dispatch dictionary pattern
- corrected import ordering
Issue: AAH-8
if resolved_version_number:
resolved_deps[key] = resolved_version_number
else:
resolved_deps[key] = f"Conflict! {deps}"
Copy link
Contributor

Choose a reason for hiding this comment

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

Response format in case of conflict is vague and requires parsing on a client.

Can you please give a link to this API format specification?

@bmclaughlin
Copy link
Contributor Author

No errors:

HTTP 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "dependencies": {
        "bmclaughlin.deps2": "0.0.3",
        "bmclaughlin.deps1": "0.0.4",
        "bmclaughlin.execution_env_test": "0.1.2",
        "ansible.netcommon": "1.3.0"
    }
}

With errors:

HTTP 200 OK
Allow: GET, HEAD, OPTIONS
Content-Type: application/json
Vary: Accept

{
    "dependencies": {
        "bmclaughlin.conflict2": "0.0.3"
    },
    "errors": {
        "bmclaughlin.deps2": {
            "reason": "Conflict",
            "constraints": ">0.1.0,<0.1.0"
        }
    }
}

@chouseknecht
Copy link
Contributor

We're going to migrate this to pulp_ansible.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
4 participants