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

Remove the v1 API #5293

Merged
merged 7 commits into from Mar 13, 2019
Merged

Remove the v1 API #5293

merged 7 commits into from Mar 13, 2019

Conversation

@davidfischer
Copy link
Contributor

@davidfischer davidfischer commented Feb 14, 2019

  • Remove the v1 API
  • No longer rely on tastypie

Open questions

  • The embed API is still under /api/v1/embed/. This does not remove that. Perhaps that endpoint should be moved or mirrored under /api/v2? The embed API does not use tastypie. Update: Moved to /api/v2/embed/...
@davidfischer davidfischer requested a review from Feb 14, 2019
@humitos humitos mentioned this pull request Feb 18, 2019
@stsewd
Copy link
Member

@stsewd stsewd commented Feb 18, 2019

Shouldn't we at least left the docs page with a warning explaining that v1 was completely removed? Maybe linking to the post or redirect to v2/v3?

@stsewd
Copy link
Member

@stsewd stsewd commented Feb 18, 2019

The embed API is still under /api/v1/embed/. This does not remove that. Perhaps that endpoint should be moved or mirrored under /api/v2? The embed API does not use tastypie.

There are some discussions about that in #5207 and #2667 (comment)

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 18, 2019

Shouldn't we at least left the docs page with a warning explaining that v1 was completely removed? Maybe linking to the post or redirect to v2/v3?

I'm not opposed to leaving the v1 doc and putting a large note there that it has been removed. It also should not be in the toctree.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 18, 2019

At the same time, I'm not sure the v1 docs are that useful. After this PR, they won't work at all.

Copy link
Member

@ericholscher ericholscher left a comment

At the same time, I'm not sure the v1 docs are that useful. After this PR, they won't work at all.

I think it's worth keeping the v1.rst around with a note that it's been removed. We don't need the old content, but it shouldn't just "disappear".

  • The embed API is still under /api/v1/embed/. This does not remove that. Perhaps that endpoint should be moved or mirrored under /api/v2? The embed API does not use tastypie.

I'm 👍 on moving it to v2. Seems simple enough, but probably another PR.

Deployment

I do wonder about rolling this out slowly, if we can. Perhaps doing a brownout of these API's at the Nginx level for a day, then a week; similar to what GH did for webhooks. That would give anyone still depending on it time to migrate before we totally kill it.

@@ -1,72 +0,0 @@
# -*- coding: utf-8 -*-
Copy link
Member

@ericholscher ericholscher Feb 19, 2019

I still use this helper. I think it should be pretty simple to adopt it to v2.

Copy link
Member

@stsewd stsewd Feb 19, 2019

ha, I use it too :(

Copy link
Member

@humitos humitos Feb 20, 2019

Yes. This command is useful when debugging production projects locally.

I'd like to find a way to keep it.

Copy link
Contributor Author

@davidfischer davidfischer Feb 20, 2019

I'll update this with an adapted version using the v2 API.

Copy link
Member

@stsewd stsewd Feb 20, 2019

We can do it in another PR, tho :)

Copy link
Contributor Author

@davidfischer davidfischer Feb 20, 2019

Nah, I think it isn't too onerous... Also, that way the single PR has the full set of changes.

Copy link
Contributor Author

@davidfischer davidfischer Feb 21, 2019

I re-added this although there are some changes. The biggest one is that in order to get all the fields from an API response, the request needs to be authenticated and the user must be staff (See this). One key field that is only returned with the ProjectAdminSerializer is the requirements_file.

By default, the management command will be unauthenticated. However, if you have READ_THE_DOCS_USERNAME and READ_THE_DOCS_PASSWORD in your environment variables when you execute the management command, they'll be used.

- This is not publicly linked
- It mentions that the API has been removed
@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 19, 2019

I do wonder about rolling this out slowly, if we can. Perhaps doing a brownout of these API's at the Nginx level for a day, then a week; similar to what GH did for webhooks. That would give anyone still depending on it time to migrate before we totally kill it.

I think this is a good idea.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 19, 2019

I think it's worth keeping the v1.rst around with a note that it's been removed. We don't need the old content, but it shouldn't just "disappear".

I re-added the page although it's blank except for noting that the API has been removed. I think that's good in case people have links to the API or whatnot but I think the content of the page ceases to have value once the API is turned off.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Feb 19, 2019

Also, the embed API has now been mirrored to https://readthedocs.org/api/v2/embed/.

Copy link
Member

@humitos humitos left a comment

Changes are good.

  • I like the idea of temporarily and gradually kill the APIv1.
  • I'd like to rewrite the management command that uses APIv1 to use APIv2. I think this is possible now that we have the ?project__slug filter on some endpoints.

)

for attribute in project_data:
if attribute not in exclude_attributes:
setattr(project, attribute, project_data[attribute])
self.stdout.write(' - Setting {key} to {val}'.format(
key=attribute,
val=project_data[attribute]),
Copy link
Member

@humitos humitos Feb 21, 2019

This one is a nice improvement!

user1 = User.objects.filter(pk__gt=0).order_by('pk').first()

if 'READ_THE_DOCS_USERNAME' in os.environ and 'READ_THE_DOCS_PASSWORD' in os.environ:
Copy link
Member

@humitos humitos Feb 21, 2019

I have a personal preference for READTHEDOCS_USERNAME (to follow the same pattern from https://docs.readthedocs.io/en/latest/builds.html#build-environment).

Copy link
Contributor Author

@davidfischer davidfischer Feb 21, 2019

I think that's pretty reasonable to stay consistent.

Originally, the Read the Docs API allowed connections over insecure HTTP.
Starting in January 2019, requests over HTTP
will be automatically redirected to HTTPS
and non-GET/HEAD requests over insecure HTTP will fail.
Copy link
Contributor Author

@davidfischer davidfischer Feb 21, 2019

I removed this while I was here, but technically this change is not due to API v1 but rather due to readthedocs/readthedocs-ops#461.

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 4, 2019

Looks like we can remove more dependencies here

https://github.com/rtfd/readthedocs.org/pull/5389/files#r262275644

@stsewd
Copy link
Member

@stsewd stsewd commented Mar 4, 2019

And guessing that we can remove some dependencies from the installation guide

https://github.com/rtfd/readthedocs.org/blob/master/docs/install.rst#L41-L42

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Mar 12, 2019

We've been returning failures for API v1 for over a week and I haven't heard anything. I think this is ok to go. I'll see if I can remove those deps.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Mar 12, 2019

I'll see if I can remove those deps.

lxml is required by pyquery but I'll update the comments and move it to a more appropriate spot in the requirements file.

@davidfischer
Copy link
Contributor Author

@davidfischer davidfischer commented Mar 12, 2019

I should also add that while defusedxml is not strictly required, I think that we should install defusedxml as long as lxml is used. DefusedXML is a library that attempts to mitigate or remove XML parsing vulnerabilities and in many cases it monkeypatches other libraries.

stsewd
stsewd approved these changes Mar 12, 2019
Copy link
Member

@stsewd stsewd left a comment

I'm dying to see our coverage after this :)

Copy link
Member

@humitos humitos left a comment

Looks good! Thanks David for taking care of this ;)

@humitos humitos merged commit 54397a0 into master Mar 13, 2019
1 check passed
@delete-merged-branch delete-merged-branch bot deleted the davidfischer/remove-apiv1 branch Mar 13, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants