-
Notifications
You must be signed in to change notification settings - Fork 158
Enable API-versioning and allow for both v3 and v4 versions. #7802
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,9 @@ | ||
| Add `/v4/` API to Pulp. | ||
|
|
||
| This adds a `/v4/` API path to Pulp, in parallel to the existing `/v3/` path. The two | ||
| are currently (nearly) identical APIs - see the `/pulp/api/v4/status/` ouput for the | ||
| only (current) end-user-visible impact. | ||
|
|
||
| This change is primarily setting the stage to allow for future API changes and growth. | ||
| It is in TECH PREVIEW, and is likely to have significant changes happening to it as we | ||
| continue integrating into the rest of the Pulp architecture. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| # Generated by Django 5.2.14 on 2026-07-10 23:34 | ||
|
|
||
| from django.db import migrations, models | ||
|
|
||
|
|
||
| class Migration(migrations.Migration): | ||
|
|
||
| dependencies = [ | ||
| ('core', '0153_taskschedule_pulp_domain_alter_taskschedule_name_and_more'), | ||
| ] | ||
|
|
||
| operations = [ | ||
| migrations.AddField( | ||
| model_name='task', | ||
| name='pulp_api_version', | ||
| field=models.TextField(default='v3'), | ||
| ), | ||
| ] |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,6 +1,7 @@ | ||
| import typing as t | ||
| from gettext import gettext as _ | ||
|
|
||
| from django.conf import settings | ||
| from drf_spectacular.types import OpenApiTypes | ||
| from drf_spectacular.utils import extend_schema_serializer | ||
| from rest_framework import serializers | ||
|
|
@@ -118,6 +119,11 @@ class TaskSerializer(ModelSerializer): | |
| help_text=_("The result of this task."), | ||
| ) | ||
|
|
||
| pulp_api_version = serializers.CharField( | ||
| help_text=_("The API-version that was invoked when creating the task."), | ||
| default=settings.REST_FRAMEWORK.get("DEFAULT_VERSION", "v3"), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Likewise, having local fallbacks kinda sucks.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Also is there a good reason to serialize this? It's useful for debugging I suppose but otherwise I'm not sure it's relevant to the user at all. We don't serialize task arguments for example.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Hm - is there a good reason to not do so?
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It just feels like an implementation detail, and I would default to not exposing those.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Mmm, I guess I see this differently - knowing how something was created is (or at some point will be, as we modify based-on-version more) important for knowing what to expect back. So to me, this is useful information for a user looking at the task.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how to resolve "you don't think it's useful and I do" :)
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Well, what is a concrete potential use for that info? I'm fine with exposing it if there's a solid case for it being useful, I just don't want to expose implementation details otherwise. Especially if it could be added back later should one arise.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I can't predict the future. I can imagine a task's method being invoked from a worker, looking at the task-context, and deciding "oh, I don't do this The Old Way, I do this the vN way", which results in a different code path/data/whatever. I don't see "what API version was I created with" to be "an implementation detail" - it's specifically public information.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's already tracked in the task arguments though (specifically IMO if we need to debug, we would just do so in the exact same way we would debug anything else. I don't see a need to special case this. All sorts of other options can trigger different codepaths within the task -
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I will probably submit a followup PR with this, but won't block this PR as-is. |
||
| ) | ||
|
|
||
| def get_worker(self, obj) -> t.Optional[OpenApiTypes.URI]: | ||
| return None | ||
|
|
||
|
|
@@ -149,13 +155,15 @@ class Meta: | |
| "created_resource_prns", | ||
| "reserved_resources_record", | ||
| "result", | ||
| "pulp_api_version", | ||
| ) | ||
|
|
||
|
|
||
| class MinimalTaskSerializer(TaskSerializer): | ||
| class Meta: | ||
| model = models.Task | ||
| fields = ModelSerializer.Meta.fields + ( | ||
| "pulp_api_version", | ||
| "name", | ||
| "state", | ||
| "unblocked_at", | ||
|
|
||
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.
Apart from
version, what arguments can end up being passed here? And what is the likelihood that any would overlap with task arguments?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.
Especially provided this applies across basically all tasks, so there are a wide variety of inputs that could potentially overlap.
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.
To address this, changed to be specific about version - and also responded to @mdellweg 's "can this be api_version" comment, because that will also reduce collision-chance.