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

Send Build Status Report Using GitHub Status API #5865

Merged

Conversation

@saadmk11
Copy link
Member

@saadmk11 saadmk11 commented Jun 28, 2019

Sending build status pending and suncess or failed using GitHub Status API.
Will extend this for other services in the future (e.g: gitlab, bitbucket)

TODO:

  • Add Tests.
@saadmk11 saadmk11 requested review from ericholscher and Jun 28, 2019
@saadmk11 saadmk11 added this to In progress in Pull Request Builder via automation Jun 28, 2019
@saadmk11 saadmk11 changed the title End Build Status Report Using GitHub Status API Send Build Status Report Using GitHub Status API Jun 28, 2019
Copy link
Member

@ericholscher ericholscher left a comment

This looks pretty simple and like a good approach. Just a few small tidbits I noticed.

readthedocs/builds/constants.py Outdated Show resolved Hide resolved
readthedocs/builds/models.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/core/utils/__init__.py Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
@@ -125,6 +128,10 @@ def prepare_build(
options['soft_time_limit'] = time_limit
options['time_limit'] = int(time_limit * 1.2)

if build:
# Send pending Build Status to Git Status API
send_build_status.delay(build.id, BUILD_STATUS_PENDING)
Copy link
Member

@stsewd stsewd Jul 2, 2019

not sure if calling this from here is the best place. I don't have a suggestion yet, but I'll try to dig the code. Also, we have signals to report the build status, maybe we can use those

Copy link
Member Author

@saadmk11 saadmk11 Jul 2, 2019

@stsewd I couldn't not find a better place for this . we should send the status report as soon as possible. please suggest me a better place when you get time :)

readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
readthedocs/oauth/services/github.py Outdated Show resolved Hide resolved
@saadmk11 saadmk11 force-pushed the github-status-api branch from 09ca1ad to 9b10ea6 Jul 4, 2019
@saadmk11 saadmk11 requested review from ericholscher, and stsewd Jul 4, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Good changes and refactors. This is getting closer 👍

I also want to test this locally, but probably won't have time today for it.

readthedocs/builds/models.py Outdated Show resolved Hide resolved
readthedocs/doc_builder/environments.py Show resolved Hide resolved
readthedocs/oauth/services/base.py Show resolved Hide resolved
self.send_build_status(
self.build['id'], BUILD_STATUS_FAILURE
)
elif self.build_env.successful:
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

What is the other state here? Should this just be an else? If not, we should do an else with a log.warning.

Copy link
Member Author

@saadmk11 saadmk11 Jul 9, 2019

@ericholscher there are no other states (BuildEnvironment) , it will succeed or fail (in the Build Environment) at this point of the build. we could add an warning if we want. but I don't think we need to :)

readthedocs/projects/tasks.py Outdated Show resolved Hide resolved
# TODO: Send build status for other providers.


def send_external_build_status(build_pk, state):
Copy link
Member

@ericholscher ericholscher Jul 8, 2019

Shouldn't this function be in utils instead of tasks?

Copy link
Member Author

@saadmk11 saadmk11 Jul 9, 2019

This will just create a circular import error, as the send_build_status task is already in the task.py file

@saadmk11 saadmk11 requested a review from ericholscher Jul 9, 2019
Copy link
Member

@ericholscher ericholscher left a comment

Looks good on my side 👍

stsewd
stsewd approved these changes Jul 9, 2019
Copy link
Member

@stsewd stsewd left a comment

Just some small things, nothing blocking 👍

tasks.send_build_status(external_build, BUILD_STATUS_SUCCESS)

send_build_status.assert_not_called()
mock_logger.info.assert_called_with(
Copy link
Member

@stsewd stsewd Jul 9, 2019

I don't think mocking the logger is needed for this test

self.assertFalse(success)
mock_logger.exception.assert_called_with(
'GitHub commit status creation failed for project: %s',
self.project,
Copy link
Member

@stsewd stsewd Jul 9, 2019

Looks like there is an extra level of indentation here

@ericholscher ericholscher merged commit c4f06dc into readthedocs:gsoc-19-pr-builder Jul 10, 2019
Pull Request Builder automation moved this from In progress to Done Jul 10, 2019
@ericholscher
Copy link
Member

@ericholscher ericholscher commented Jul 10, 2019

🎉

@saadmk11 saadmk11 deleted the github-status-api branch Jul 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

3 participants