Auto Build Documentation on Pull Request (PR Builder) #5828
Conversation
This is just an initial proof of concept. We still need to update the External type properly, and do lots of other work. This is just a start to show how we can sync and build them.
saadmk11
added
the
PR: work in progress
saadmk11
changed the title
ericholscher
left a comment
ericholscher
left a comment
There was a problem hiding this comment.
This looks like a good start. Thinking more about this, we probably need to figure out a better flow for syncing an incoming PR version. If we could create the Version then, and trigger a build for it, that seems like a better approach. We still likely want to sync it via VCS as well though, but I'm not 100% sure :/
readthedocs/api/v2/utils.py
Outdated
| verbose_name=version_name, | ||
| ) | ||
| added.add(created_version.slug) | ||
|
|
There was a problem hiding this comment.
Isn't this line the same as the line below it? Why do we need this extra logic?
readthedocs/api/v2/utils.py
Outdated
| version.active = True | ||
| version.save() | ||
|
|
||
| trigger_build(project=project, version=version) |
There was a problem hiding this comment.
We definitely don't want to trigger all builds for external versions. This will create hundreds of builds on first import. We need a better mechanism for this I think.
| return self.sync_versions(self.project) | ||
|
|
||
| if event == GITHUB_PULL_REQUEST: | ||
| return self.sync_versions(self.project) |
There was a problem hiding this comment.
This might be the place where we want to trigger a build after syncing. I'm not 100% sure though.
There was a problem hiding this comment.
We might need to think a bit more about how we're doing version syncing actually. I think maybe in this case we can just create the version via the incoming PR and trigger the build. That way we don't need to clone the repo 2x.
saadmk11
left a comment
saadmk11
left a comment
There was a problem hiding this comment.
I left some comments to explain why I did those. It would be great to have review
| def get_external_version_data(self): | ||
| """Get Commit Sha and pull request number from payload""" | ||
| identifier = self.data['pull_request']['head']['sha'] | ||
| verbose_name = str(self.data['number']) |
There was a problem hiding this comment.
here I'm considering PR_Number as verbose name.
| external_version = get_or_create_external_version( | ||
| self.project, identifier, verbose_name | ||
| ) | ||
| return self.get_response_push(self.project, external_version.verbose_name) |
There was a problem hiding this comment.
To use the pre-written self.get_response_push() we can not pass a version instance so here I passed the verbose name but I think it will be much better if we pass the instance. to do this we need to create another function and not use self.get_response_push() we might use that function to send triggered status on the PR using the Github Status API. I would like to get you thought on this :)
There was a problem hiding this comment.
Yea, we can use whatever makes sense. If it's doing something different than existing code, don't feel bad coming up with a new method.
| return self.get_response_push(self.project, external_version.verbose_name) | ||
|
|
||
| except KeyError: | ||
| raise ParseError('Parameters "sha" and "number" are required') |
There was a problem hiding this comment.
The Error message can be changed
| def get_or_create_external_version(project, identifier, verbose_name): | ||
| external_version = project.versions(manager=EXTERNAL).filter(verbose_name=verbose_name).first() | ||
| if external_version: | ||
| if external_version.identifier != identifier: |
There was a problem hiding this comment.
This will be updated when there is a new commit in the PR
There was a problem hiding this comment.
This is probably better in a code comment, not a PR comment that will be lost :)
| ) | ||
| version_repo = self.get_vcs_repo() | ||
| version_repo.update() | ||
| version_repo.update(version=self.version) |
There was a problem hiding this comment.
we need to pass the version here so that we can determine which PR should be fetched. there previous implementation fetched all the PR's all together regardless it was open or closed
There was a problem hiding this comment.
Seems like a good and small change 👍
|
|
||
| if ( | ||
| event == GITHUB_PULL_REQUEST and | ||
| self.data['action'] in [GITHUB_PULL_REQUEST_OPEN, GITHUB_PULL_REQUEST_SYNC] |
There was a problem hiding this comment.
This will handle both PR opened and synchronized (new commit in the PR) events.
There was a problem hiding this comment.
Tested it locally and it worked great 👍
|
This approach looks good to me, so far! |
ericholscher
left a comment
ericholscher
left a comment
There was a problem hiding this comment.
Good to merge into the feature branch 👍
2 participants
This is the PR which will have all the initial code for the PR Builder.
There are lots of stuff that need to be done.
ref: #1340 #5684