Add S-needs-code-changes if travis build fails #88
Conversation
|
Heads up! This PR modifies the following files:
|
|
|
||
| # If more functionality is needed from this class, it might be a | ||
| # better decision to use travispy | ||
| class TravisCiApiProvider(): |
There was a problem hiding this comment.
Why can't we have this along with the other API provider classes in newpr.py?
There was a problem hiding this comment.
I actually prefer this to live separately.
|
The relevant tests should also be updated, btw. |
|
|
||
|
|
||
| def get_log(self, build_data): | ||
| return urllib.urlopen(self.log_url.format(job_id=self._get_job_id(build_data))).read() |
|
Great work! Let's go ahead and remove any unused code from the Travis API provider. |
|
@wafflespeanut so I need to add tests for the Tavis CI Api ? |
|
Yep! They're offline tests (very similar to what we already have) |
| return urllib.urlopen(self.log_url.format(job_id=self._get_job_id(build_data))).read() | ||
|
|
||
| url = self.build_url.format(build_id=build_id) | ||
| return json.loads(urllib.urlopen(url).read()) |
There was a problem hiding this comment.
To support testing, this should use self.get_page_content instead of urllib directly (see https://github.com/servo/highfive/blob/master/newpr.py#L201).
|
So I need to test |
| @@ -33,4 +33,9 @@ def on_pr_closed(self, api, payload): | |||
| if payload['pull_request']['merged']: | |||
| api.remove_label("S-awaiting-merge") | |||
|
|
|||
| def on_build_status(self, api, payload): | |||
| if payload['context'] == 'continuous-integration/travis-ci/pr': | |||
| if payload['state'] == 'failure' or payload['state'] == 'error': | |||
There was a problem hiding this comment.
What is the difference between failure and error with Tavis?
There was a problem hiding this comment.
I assume one of them means an internal error occurred, rather than an error during the build process, but I'm not sure. Probably best to leave both right now.
|
☔ The latest upstream changes (presumably #111) made this pull request unmergeable. Please resolve the merge conflicts. |
|
I sincerely apologize for forgetting about this. I'll review this right now and rebase it. |
| # If more functionality is needed from this class, it might be a | ||
| # better decision to use travispy | ||
| class TravisCiApiProvider(): | ||
| host_url = 'https://api.travis-ci.org' |
There was a problem hiding this comment.
We'll probably need to make this configurable somehow so we could use file:// urls for tests.
There was a problem hiding this comment.
@jdm ok, I will change that, but how can I create test for this?
There was a problem hiding this comment.
We would have a json file that contains a travis payload instead of a github payload like existing tests, and we would check for the presence of the S-needs-code-changes label in the expected output.
Add support for automatically labelling new PRs
Fix #28.
Should I remove the unused functions in travisciapiprovider.py?