Enable API-versioning and allow for both v3 and v4 versions.#7802
Enable API-versioning and allow for both v3 and v4 versions.#7802ggainey wants to merge 1 commit into
Conversation
75e4f88 to
fcdcf5b
Compare
d38417a to
1d45f72
Compare
|
|
||
| result = models.JSONField(default=None, null=True, encoder=DjangoJSONEncoder) | ||
|
|
||
| version = models.TextField(default=REST_FRAMEWORK.get("DEFAULT_VERSION", "v3")) |
There was a problem hiding this comment.
"version" as an attribute is "magic" at the DRF level - it's what gets filled in by the content of the <str:version> slug in the urlpattern. I debated on exactly this - but making it "version" means it's filled directly by the incoming **kwargs to the view, naming it anything else means calling it out specifically. I could be convinced of either.
There was a problem hiding this comment.
I was just abit afraid that we could be confusing here. And changing a database column name later is a pain.
(Pulp is in the domain for versioning on all levels. Pulp-version, pulp-api-version, repository-version, deb-version, rpm-version, ansible-collection-version, ...)
There was a problem hiding this comment.
Made the change to api_version - it bent some of the "magic" connections, but I think clarity is more important.
|
|
||
| version = serializers.CharField( | ||
| help_text=_("The API-version that was invoked when creating the task."), | ||
| default=REST_FRAMEWORK.get("DEFAULT_VERSION", "v3"), |
There was a problem hiding this comment.
| default=REST_FRAMEWORK.get("DEFAULT_VERSION", "v3"), |
I don't think we need this, tasks will never be created via this serializer.
There was a problem hiding this comment.
Possibly so, will dig a bit.
There was a problem hiding this comment.
I don't think we need this, tasks will never be created via this serializer.
You would think, but I remember there being some surprising ways and places in which the serializers were getting initialized.
| _current_user_func = ContextVar("current_user", default=lambda: None) | ||
| _current_domain = ContextVar("current_domain", default=None) | ||
| x_task_diagnostics_var = ContextVar("x_profile_task") | ||
| _current_pulp_version = ContextVar( |
There was a problem hiding this comment.
Comment: Is this really how we want to "know" what pulp-version we are operating under? Author, please review!
(also - even if we keep this, name needs to be _current_pulp_api_version)
There was a problem hiding this comment.
That I kind ow liked. It's non-invasive, yet omnipresent inside the task code.
There was a problem hiding this comment.
@ggainey Did you mean to rename this? I agree that it should be renamed and it looks like you intended to do so
|
RE ENABLE_V4_API : I am going to turn it on in this PR, in order to make sure CI runs clean with it enabled. BEFORE WE MERGE, we will turn it OFF, and let ppl who want to experiment "opt-in" to it, for several releases. |
949527d to
147579e
Compare
| ) | ||
| @action(detail=True, methods=["post"], serializer_class=FileRepositorySyncURLSerializer) | ||
| def sync(self, request, pk): | ||
| def sync(self, request, pk, **kwargs): |
There was a problem hiding this comment.
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.
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.
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.
| if not task.version: | ||
| vers_token = _current_pulp_version.set(REST_FRAMEWORK.get("DEFAULT_VERSION", "v3")) | ||
| else: | ||
| vers_token = _current_pulp_version.set(task.version) |
There was a problem hiding this comment.
So, is this a correct description
"The task needs to know about the API version it was spawned from because the task might use the serializers internally, and the serializers need to know what API version they are operating from"
and
"if the task didn't need to potentially provide that data to Serializers, the context var (within tasks) and the field on the task model would be needless"
| path("datarepair/7272/", DataRepair7272View.as_view()), | ||
| path( | ||
| "orphans/cleanup/", | ||
| re_path(r"^login/$", LoginViewSet.as_view()), |
There was a problem hiding this comment.
What purpose does regex matching serve for these simple paths?
There was a problem hiding this comment.
"Consistency", mostly. I can do one last experiment where I'm more selective about that.
7f2b196 to
5308d95
Compare
| from pulpcore.app.models.fields import EncryptedJSONField | ||
| from pulpcore.app.models.status import AppStatus | ||
| from pulpcore.app.role_util import get_users_with_perms | ||
| from pulpcore.app.settings import REST_FRAMEWORK |
There was a problem hiding this comment.
This needs to be from django.conf import settings.
2920117 to
0fd5df3
Compare
ENABLE_V4_API is now |
| "optimize": optimize, | ||
| } | ||
| if kwargs.get("version"): | ||
| task_kwargs["api_version"] = kwargs.get("version") |
There was a problem hiding this comment.
Would it be worth making this even more specific, pulp_api_version or similar? To fully minimize the risk of overlap.
One could potentially imagine passing an api_version field into a sync task for example.
There was a problem hiding this comment.
We could - but it feels like overkill? We own this code, and "api version" is WAY more likely to be "Pulp". The edge-case would be adding api-version for...someting else?
There was a problem hiding this comment.
Perhaps but I'm in favor of overkill if it's just a matter of naming and has no other downsides. It also just feels like maintaining consistency with e.g. pulp_domain
There was a problem hiding this comment.
I found an even-better reason to change to pulp_api_version: https://github.com/pulp/pulpcore/blob/main/pulpcore/app/serializers/domain.py#L295
There was a problem hiding this comment.
That is indeed a pretty good reason!
(well, it's not a direct conflict, but it's an example of potential overlap issues)
|
|
||
| result = models.JSONField(default=None, null=True, encoder=DjangoJSONEncoder) | ||
|
|
||
| api_version = models.TextField(default=settings.REST_FRAMEWORK.get("DEFAULT_VERSION", "v3")) |
There was a problem hiding this comment.
Can we not rely on DEFAULT_VERSION existing? I'd prefer that be reliable and we not need a local default fallback here.
There was a problem hiding this comment.
My thinking here (and in the case below) is that I'd rather Absolutely Positively Guarantee we get something here, than have code that wants to use api-version have to deal with it maybe being None.
There was a problem hiding this comment.
If it was somehow None, then that would be a misconfiguration, and I'd personally rather that failure be more obvious than have the behavior be dependent on a dozen local fallbacks which might accidentally drift apart over time. It introduces ways for inconsistency to creep in.
|
|
||
| api_version = serializers.CharField( | ||
| help_text=_("The API-version that was invoked when creating the task."), | ||
| default=settings.REST_FRAMEWORK.get("DEFAULT_VERSION", "v3"), |
There was a problem hiding this comment.
Likewise, having local fallbacks kinda sucks.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Hm - is there a good reason to not do so?
There was a problem hiding this comment.
It just feels like an implementation detail, and I would default to not exposing those.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
I'm not sure how to resolve "you don't think it's useful and I do" :)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
It's already tracked in the task arguments though (specifically Task.enc_kwargs). Strictly speaking it doesn't even need to be a separate field on the Task model, much less the serializer.
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 - mirror already being mentioned, but also the likes of skip_types, optimize, x-task-diagnostics (which is probably the most similar case), etc. We don't expose those publicly.
There was a problem hiding this comment.
I will probably submit a followup PR with this, but won't block this PR as-is.
| else: | ||
| data["api_version"] = request.version | ||
| data["supported_api_versions"] = settings.REST_FRAMEWORK["ALLOWED_VERSIONS"] | ||
| serializer = V4StatusSerializer(data, context=context) |
There was a problem hiding this comment.
Do we need two different versions of StatusSerializer or can we just alter the behavior dynamically?
There was a problem hiding this comment.
There are multiple ways to deal with versioning - this is intended as "an example". Altering "inside of" the view/serializer/model is another one, absolutely.
| "deferred": deferred, | ||
| "profile_options": x_task_diagnostics_var.get(None), | ||
| "app_lock": app_lock, | ||
| "api_version": kwargs.get("version", default_vers) if kwargs else default_vers, |
There was a problem hiding this comment.
This code could check the context variable, thus making it automatically self-propagating should a task be spawned from another task.
There was a problem hiding this comment.
I will submit a followup PR with this.
This is a tech-preview PR, and will only "turn on" if the ENABLE_V4_API is set to True in settings. It will allow urls for /v3/ and /v4/, and reject other versions. For urlpatterns registered with a `/<str:version>/` slug, views will be called with a `version=` kwarg. NOTE: To play this game, plugins will need to insure that all views accept `**kwargs` first, and then update the patterns in their `urls.py` to include the version-slug. See the `/status/` view and serializer(s) for an example of how to respond based on the incoming request. NOTE to implementers: **existing tests must pass without changes** whether the ENABLE flag is True or False. If that isn't the case - you're doing something wrong. closes pulp#6462
|
@ggainey @mdellweg What do you think about these tweaks? ggainey#275 (they can be considered and taken separately if needed) |
This is a tech-preview PR, and will only "turn on" if the ENABLE_V4_API is set to True in settings.
It will allow urls for /v3/ and /v4/, and reject other versions.
For urlpatterns registered with a
/<str:version>/slug, views will be called with aversion=kwarg.NOTE: To play this game, plugins will need to insure that all views accept
**kwargsfirst, and then update the patterns in theirurls.pyto include the version-slug.See the
/status/view and serializer(s) for an example of how to respond based on the incoming request.NOTE to implementers: existing tests must pass without changes whether the ENABLE flag is True or False. If that isn't the case - you're doing something wrong.
closes #6462