Fix checking of PR status #10085

ericholscher · 2023-02-28T21:44:36Z

This was previously checking the status of the build object,
but I think we want to check the status that is passed in,
and only link to the built docs when the build was successful.

Fixes #10018

This was previously checking the status of the build object, but I think we want to check the status that is passed in, and only link to the built docs when the build was successful.

ericholscher · 2023-02-28T21:46:08Z

readthedocs/oauth/services/github.py


-        if build.state in BUILD_FINAL_STATES and build.success:
+        if status == BUILD_STATUS_SUCCESS:


@humitos I think this is actually what we want? Basically just check if the status was success, then link to the built docs?

Yeah, I think so 😅

benjaoming

Makes sense, BUILD_STATUS_SUCCESS != BUILD_STATE_FINISHED - the previous implementation wouldn't find its way to get_absolute_url() on "success" because it compared it with a build state "finished" 👍

I like the attempt to differentiate between "state" and "status", but I'm not advanced enough in the English language to know what the difference would be.. but I get that it has different definitions in the code base 👍

benjaoming · 2023-03-07T14:40:31Z

test failures seem legit

humitos · 2023-03-07T15:00:33Z

I like the attempt to differentiate between "state" and "status", but I'm not advanced enough in the English language to know what the difference would be

He, we had have some talks about this and I dislike these words because for me they are synonymous 😅 . Besides, they also look pretty similar at first sight and I never know what each of them mean without context. In any case, this topic is un-related from this PR, but just wanted to make my own note about this 😄

ericholscher · 2023-03-07T19:31:19Z

I like the attempt to differentiate between "state" and "status", but I'm not advanced enough in the English language to know what the difference would be

He, we had have some talks about this and I dislike these words because for me they are synonymous 😅 . Besides, they also look pretty similar at first sight and I never know what each of them mean without context. In any case, this topic is un-related from this PR, but just wanted to make my own note about this 😄

I was mostly just cleaning up the usage, since the code was using them interchangably, as well 🙃

ericholscher · 2023-03-07T19:31:31Z

I'm not 100% sure whats happening on tests, will have to dive into that a bit deeper.

ericholscher · 2023-03-14T21:30:05Z

I'm holding off on this, because it's not a huge priority, and my sprint is getting slammed. If anyone is excited to finish this up, feel free to get the tests passing.

benjaoming · 2023-04-22T18:44:55Z

Testing this on a local setup with GitHub OAuth and webhook delivery to local development environment... but getting TypeError: send_build_status() got an unexpected keyword argument 'build'

web_1          | Internal Server Error: /api/v2/webhook/tutorial-template/1/
web_1          | Traceback (most recent call last):
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/exception.py", line 47, in inner
web_1          |     response = get_response(request)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/core/handlers/base.py", line 181, in _get_response
web_1          |     response = wrapped_callback(request, *callback_args, **callback_kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
web_1          |     return view_func(*args, **kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 70, in view
web_1          |     return self.dispatch(request, *args, **kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 509, in dispatch
web_1          |     response = self.handle_exception(exc)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 469, in handle_exception
web_1          |     self.raise_uncaught_exception(exc)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
web_1          |     raise exc
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 506, in dispatch
web_1          |     response = handler(request, *args, **kwargs)
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/api/v2/views/integrations.py", line 873, in post
web_1          |     return view(request._request, project_slug)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/views/decorators/csrf.py", line 54, in wrapped_view
web_1          |     return view_func(*args, **kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/django/views/generic/base.py", line 70, in view
web_1          |     return self.dispatch(request, *args, **kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 509, in dispatch
web_1          |     response = self.handle_exception(exc)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 469, in handle_exception
web_1          |     self.raise_uncaught_exception(exc)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 480, in raise_uncaught_exception
web_1          |     raise exc
web_1          |   File "/usr/local/lib/python3.10/dist-packages/rest_framework/views.py", line 506, in dispatch
web_1          |     response = handler(request, *args, **kwargs)
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/api/v2/views/integrations.py", line 108, in post
web_1          |     resp = self.handle_webhook()
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/api/v2/views/integrations.py", line 470, in handle_webhook
web_1          |     return self.get_external_version_response(self.project)
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/api/v2/views/integrations.py", line 247, in get_external_version_response
web_1          |     to_build = build_external_version(
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/core/views/hooks.py", line 215, in build_external_version
web_1          |     trigger_build(project=project, version=version, commit=version.identifier)
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/core/utils/__init__.py", line 196, in trigger_build
web_1          |     update_docs_task, build = prepare_build(
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/core/utils/__init__.py", line 105, in prepare_build
web_1          |     send_external_build_status(
web_1          |   File "/usr/src/app/checkouts/readthedocs.org/readthedocs/projects/tasks/utils.py", line 154, in send_external_build_status
web_1          |     send_build_status.delay(build=build_pk, commit=commit, status=status)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/celery/app/task.py", line 425, in delay
web_1          |     return self.apply_async(args, kwargs)
web_1          |   File "/usr/local/lib/python3.10/dist-packages/celery/app/task.py", line 540, in apply_async
web_1          |     check_arguments(*(args or ()), **(kwargs or {}))
web_1          | TypeError: send_build_status() got an unexpected keyword argument 'build'

…args

benjaoming · 2023-04-22T19:18:12Z

Tested that using positional arguments for send_build_status works now ✔️

I'm suspecting that Celery validates arguments differently from how Python can use positional arguments by name.

…ify-status-checking

benjaoming

Judging from local testing, we need to use positional arguments because of Celery.

I fixed the tests to match this.

If there's anything else you'd like to add, LMK.. I can test locally.

benjaoming · 2023-04-26T16:19:19Z

Just verified the last thing that came cross my mind: All other services with a send_build_status use positional arguments as well 👍 ✔️

benjaoming · 2023-05-01T21:50:55Z

Auto-merge vs. merge vs. linting 🙃

Fix checking of PR status

e10a454

This was previously checking the status of the build object, but I think we want to check the status that is passed in, and only link to the built docs when the build was successful.

ericholscher requested a review from a team as a code owner February 28, 2023 21:44

ericholscher requested a review from benjaoming February 28, 2023 21:44

auto-assign bot assigned ericholscher Feb 28, 2023

ericholscher commented Feb 28, 2023

View reviewed changes

benjaoming approved these changes Mar 7, 2023

View reviewed changes

use positional arguments, doesn't seem like celery can use them as kw…

75b2a0d

…args

benjaoming added a commit to benjaoming/tutorial-template that referenced this pull request Apr 22, 2023

Test readthedocs/readthedocs.org#10085 again

a9d58cf

benjaoming added 2 commits April 22, 2023 21:19

Merge branch 'main' of github.com:readthedocs/readthedocs.org into un…

e64677c

…ify-status-checking

Revert remaining kwargs calls to positional args

309cd3f

benjaoming approved these changes Apr 22, 2023

View reviewed changes

Merge branch 'main' into unify-status-checking

f789c94

ericholscher enabled auto-merge (squash) May 1, 2023 21:40

hello darker my old friend

ef5ea62

ericholscher merged commit 5f153d9 into main May 1, 2023

ericholscher deleted the unify-status-checking branch May 1, 2023 22:07

benjaoming mentioned this pull request May 30, 2023

Skipped PR build remains a pending check in GitHub UI #10364

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix checking of PR status #10085

Fix checking of PR status #10085

ericholscher commented Feb 28, 2023 •

edited

Loading

ericholscher Feb 28, 2023

humitos Feb 28, 2023

benjaoming left a comment

benjaoming commented Mar 7, 2023

humitos commented Mar 7, 2023

ericholscher commented Mar 7, 2023

ericholscher commented Mar 7, 2023

ericholscher commented Mar 14, 2023

benjaoming commented Apr 22, 2023

benjaoming commented Apr 22, 2023

benjaoming left a comment

benjaoming commented Apr 26, 2023

benjaoming commented May 1, 2023


		if build.state in BUILD_FINAL_STATES and build.success:
		if status == BUILD_STATUS_SUCCESS:

Fix checking of PR status #10085

Fix checking of PR status #10085

Conversation

ericholscher commented Feb 28, 2023 • edited Loading

ericholscher Feb 28, 2023

Choose a reason for hiding this comment

humitos Feb 28, 2023

Choose a reason for hiding this comment

benjaoming left a comment

Choose a reason for hiding this comment

benjaoming commented Mar 7, 2023

humitos commented Mar 7, 2023

ericholscher commented Mar 7, 2023

ericholscher commented Mar 7, 2023

ericholscher commented Mar 14, 2023

benjaoming commented Apr 22, 2023

benjaoming commented Apr 22, 2023

benjaoming left a comment

Choose a reason for hiding this comment

benjaoming commented Apr 26, 2023

benjaoming commented May 1, 2023

ericholscher commented Feb 28, 2023 •

edited

Loading