remove /api/v1 and deprecated credential fields #3413
Conversation
ryanpetrello
force-pushed
the
bye-bye-v1
branch
2 times, most recently
from
1275e94
to
edef1e9
Compare
|
Build failed.
|
| if field_name in V1Credential.FIELDS: | ||
| return self.build_standard_field(field_name, | ||
| V1Credential.FIELDS[field_name]) | ||
| return super(V1CredentialFields, self).build_field(field_name, info, model_class, nested_depth) |
There was a problem hiding this comment.
I'll second that. This was the worst 😂
| @@ -6,4 +6,4 @@ One result should be returned containing the following fields: | |||
|
|
|||
| {% include "api/_result_fields_common.md" %} | |||
|
|
|||
| Use the primary URL for the user (/api/v1/users/N/) to modify the user. | |||
| Use the primary URL for the user (/api/v2/users/N/) to modify the user. | |||
There was a problem hiding this comment.
well this wasn't ideal documentation. We could peel some of these off for sooner merge.
| @@ -23,7 +22,6 @@ | |||
| urls = [ | |||
| url(r'^$', JobList.as_view(), name='job_list'), | |||
| url(r'^(?P<pk>[0-9]+)/$', JobDetail.as_view(), name='job_detail'), | |||
| url(r'^(?P<pk>[0-9]+)/start/$', JobStart.as_view(), name='job_start'), # Todo: Remove In 3.3 | |||
| # the given list without merging with JT credentials | ||
| if prompted_value: | ||
| obj._deprecated_credential_launch = True # signal to not merge credentials | ||
| new_credentials.extend(prompted_value) |
There was a problem hiding this comment.
this removal of the loop looks good on first read. It'll need some more attention when we get closer to merge.
| @@ -3412,56 +3349,17 @@ class SystemJobTemplateNotificationTemplatesSuccessList(SubListCreateAttachDetac | |||
| relationship = 'notification_templates_success' | |||
|
|
|||
|
|
|||
| class JobList(ListCreateAPIView): | |||
| class JobList(ListAPIView): | |||
| @@ -2853,13 +2799,6 @@ class JobTemplateJobsList(SubListCreateAPIView): | |||
| relationship = 'jobs' | |||
| parent_key = 'job_template' | |||
There was a problem hiding this comment.
JobTemplateJobsList(SubListCreateAPIView)--> sublist view, as with jobs list
| 'Group "%s" from v1 API is not deleted by overwrite', | ||
| self.inventory_source.deprecated_group.name | ||
| ) | ||
| del_group_pks.discard(self.inventory_source.deprecated_group_id) |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
recheck |
|
Build failed.
|
| def get_queryset(self): | ||
| # TODO: remove after the depreciation of v1 API | ||
| qs = super(UnifiedJobTemplateAccess, self).get_queryset() | ||
| return qs.exclude(inventorysource__source="") |
There was a problem hiding this comment.
We should remove all inventory sources with source="" through a data migration, and remove empty string from the choices for the source field. (Django might help you auto-generate a migration file after you make the change to the choices)
I've been turning this over in my mind, and it just doesn't make any sense to keep them.
|
Build failed.
|
|
Build failed.
|
|
Build succeeded.
|
awx/api/serializers.py
Outdated
| else: | ||
| return ['copy', 'edit', 'delete'] | ||
| def show_capabilities(self): | ||
| return ['copy', 'edit', 'delete'] |
There was a problem hiding this comment.
This, as well as the other similar method, no longer needs to be a @property it can just be
show_capabilities = ['copy', 'edit', 'delete']
|
Build failed.
|
|
Build succeeded.
|
ryanpetrello
force-pushed
the
bye-bye-v1
branch
2 times, most recently
from
60d3d97
to
e66da4c
Compare
|
Not sure where this is going on but this is a bug: GIVEN a cred that has "prompt on launch" associated with a JT, But right now you are able to |
|
Build succeeded.
|
| summary_fields['extra_credentials'] = extra_creds | ||
| summary_fields['credentials'] = all_creds | ||
| if self.is_detail_view: | ||
| summary_fields['extra_credentials'] = extra_creds |
There was a problem hiding this comment.
Since you removed the credential field, I don't know if I agree with providing extra_credentials in the summary fields (I know there's a lot of special handling in the job launch view too). Aren't these both parts of the same compatibility shim?
I do see extra_credentials referenced in javascript, which does seem like a problem.
So I'm still lacking an idea of exactly which fields are being removed, and why.
There was a problem hiding this comment.
hmm... going back and fourth with the job launch view, I think I'm seeing better the intentionality of this. I'm inferring that extra_credentials was maybe introduced later than credential and vault_credential, and gives cloud credentials in general.
There was a problem hiding this comment.
I've kind of gone back and forth on this, and my rationale here was that the extra_credentials interface is considerably more recent than the the credential and vault_credential polyfills (cir. Ansible Tower 3.3, providing support for multiple cloud/network credentials vs Ansible Tower 3.2).
I would like to remove extra_credentials (it's been deprecated for some time), but I think it would be more responsible to keep it around for awhile longer.
|
@kdelee I agree that looks like a bug, is it specific to this branch, or is it pre-existing? I think I'm about done with this, but that seems like 1 open question that's still hanging around. |
|
@AlanCoding looking more into it looks like might be bug on my side of how I am associating credentials |
| 'id': cred.pk, | ||
| 'name': cred.name, | ||
| 'description': cred.description, | ||
| 'kind': cred.kind, |
There was a problem hiding this comment.
we will most likely be removing kind from credentials in the near future but we haven't done so yet - so i've included the kind field in the summary to keep it consistent with how we represent summarized credentials elsewhere.
|
Build succeeded.
|
|
Build succeeded.
|
Currently, the credentials list doesn't seem to be returning any options data for 'kind' so this code wasn't being reached. In the future api updates, we'll also be removing the 'kind' field from credentials in general.
|
Build failed.
|
|
recheck |
|
Build succeeded.
|
|
Build succeeded (gate pipeline).
|
No description provided.