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

Add logic to filter PRs based on the last commit status #101

Merged
merged 16 commits into from
Sep 27, 2013

Conversation

sbesson
Copy link
Member

@sbesson sbesson commented Sep 25, 2013

This PR adds some logic to filter PRs by status namely

  • scc merge <branch> --check-commit-status excludes PRs which status attached to the last commit is not strictly success
  • a corresponding integration test testStatus is added to TestMerge where a PR is opene, its status is modified (none, pending, failure and success) and the outcome of scc merge --check-commit-status is checked for each status

As part of these changes, the following changes have been brought:

  • the PullRequest object in scc.py has been refactored and minimally acts as a wrapper of the github.PullRequest object. New methods have been added to retrieve the corresponding issue, last commit and last status. The construction changes have been applied to the rest of the code
  • the TestMerge integration tests have been rewritte: a new PR is created for each test and the outcome of the scc merge command is systematically checked for each test
  • the tests have been split under test/unit and test/integration. At the moment only unit tests are run by the Travis build (dropping all the Github part) and the entire test suite is expected to be run under hudson.

/cc @mtbc, @joshmoore

Immediate build candidates for applying these changes once merged are the documentation builds where the Travis output is very reliable.

- Modify the PullRequest() constructor to build from github.PullRequest
- Modify the PullRequest() object creation across the code
- Add get_issue() method to retrieve the issue associated to the PR
- Add get_last_commit() method to retrieve the head commit using the sha1
- Add get_last_status() method to retrieve the last status of the last comment
This commit defines a new flag argument --check-commit-status for all commands
derived from the FilteredPullRequestCommand abstract class. The
find_candidates() method is updated to add an extra filtering step if this
flag is passed.
This commit provides the default framework to close ome#41. Extra granularity on
the status filters can be implemented when necessary.
- Create new branch/PR for each test
- Add isMerged() method to TestMerged
- Combine remote tests
@joshmoore
Copy link
Member

Testing on dev_4_4 where ome/openmicroscopy#1499 should not be merged (as of today)....

2013-09-26 09:21:39,301 [   scc.merge] INFO  Merged PRs:
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1126 joshmoore 'Fix long-missing DELETE EventLogs'
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1430 mtbc 'fix #10564: include wells in plate run processing'
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1450 ximenesuk 'Update log jars'
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1459 will-moore 'Scripts menu 11434'
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1478 joshmoore 'Session uuid fixes'
2013-09-26 09:21:39,301 [   scc.merge] INFO    # PR 1486 melissalinkert 'Add ways to remove 0-byte pyramid files'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1528 mtbc 'Wrap event log upsert SQL in try/catch (see 10181)'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1532 joshmoore 'Unwrap namespace'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1534 jburel 'Dmrefresh'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1545 aleksandra-tarkowska '6850 loading tile in big image viewer (rebased onto dev_4_4)'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1547 jburel 'Default group'
2013-09-26 09:21:39,302 [   scc.merge] INFO    # PR 1548 snoopycrimecop 'Update dev_4_4 submodules'

looks good, but I do wonder if we don't need a section here for the status-excluded PRs as with the conflict-excluded PRs. Would that be possible?

Also add the returned log to the info log generated by the `scc merge` and
`scc set-commit-status` commands
@sbesson
Copy link
Member Author

sbesson commented Sep 26, 2013

With the latest commit, scc merge output should read like:

sebastien@sbesson:openmicroscopy (dev_4_4) $ python ~/code/ome/scc/scc.py merge -S dev_4_4 --info
2013-09-26 11:38:15,052 [   scc.merge] INFO  Finding PR based on dev_4_4 opened by any public member of the organization
2013-09-26 11:38:15,052 [   scc.merge] INFO  Including PR labelled as: include
2013-09-26 11:38:15,052 [   scc.merge] INFO  Excluding PR labelled as: exclude
2013-09-26 11:38:15,052 [   scc.merge] INFO  Excluding PR with no status or unsuccessful status
2013-09-26 11:39:38,363 [   scc.merge] INFO  Repository: openmicroscopy/openmicroscopy
2013-09-26 11:39:38,363 [   scc.merge] INFO  Status-excluded PRs:
2013-09-26 11:39:38,363 [   scc.merge] INFO    # PR 1537 mtbc 'fix #11465: add validation checks for admin service'
2013-09-26 11:39:38,363 [   scc.merge] INFO    # PR 1528 mtbc 'Wrap event log upsert SQL in try/catch (see 10181)'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1499 bpindelski 'Fix intermittent omero.LockTimeout-s in OmeroJava tests (see #11455)'
2013-09-26 11:39:38,364 [   scc.merge] INFO  Candidate PRs:
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1126 joshmoore 'Fix long-missing DELETE EventLogs'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1430 mtbc 'fix #10564: include wells in plate run processing'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1450 ximenesuk 'Update log jars'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1459 will-moore 'Scripts menu 11434'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1478 joshmoore 'Session uuid fixes'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1486 melissalinkert 'Add ways to remove 0-byte pyramid files'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1532 joshmoore 'Unwrap namespace'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1534 jburel 'Dmrefresh'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1545 aleksandra-tarkowska '6850 loading tile in big image viewer (rebased onto dev_4_4)'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1547 jburel 'Default group'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 1548 snoopycrimecop 'Update dev_4_4 submodules'
2013-09-26 11:39:38,364 [   scc.merge] INFO  
2013-09-26 11:39:38,364 [   scc.merge] INFO  Repository: openmicroscopy/bioformats
2013-09-26 11:39:38,364 [   scc.merge] INFO  Status-excluded PRs:
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 712 sbesson 'Fix hyperlink to Spring main page'
2013-09-26 11:39:38,364 [   scc.merge] INFO  Candidate PRs:
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 673 melissalinkert 'Make configuration file suffix configurable'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 707 melissalinkert 'Metamorph image names'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 710 melissalinkert 'Data test updates (rebased onto dev_4_4)'
2013-09-26 11:39:38,364 [   scc.merge] INFO    # PR 711 melissalinkert 'ImageConverter: fix dimensions of the last tile'
2013-09-26 11:39:38,365 [   scc.merge] INFO  
2013-09-26 11:39:38,365 [   scc.merge] INFO  Repository: ome/scripts
2013-09-26 11:39:38,365 [   scc.merge] INFO  Status-excluded PRs:
2013-09-26 11:39:38,365 [   scc.merge] INFO    # PR 45 will-moore 'Export big images as jpeg 10841 dev 4 4'
2013-09-26 11:39:38,365 [   scc.merge] INFO  Candidate PRs:
2013-09-26 11:39:38,365 [   scc.merge] INFO  
2013-09-26 11:39:38,365 [   scc.merge] INFO  Repository: openmicroscopy/ome-documentation
2013-09-26 11:39:38,365 [   scc.merge] INFO  Status-excluded PRs:
2013-09-26 11:39:38,365 [   scc.merge] INFO    # PR 490 sbesson 'Define Spring hyperlink and extlink for Spring doc in OMERO conf.py'
2013-09-26 11:39:38,365 [   scc.merge] INFO  Candidate PRs:
2013-09-26 11:39:38,365 [   scc.merge] INFO    # PR 488 sbesson 'Add `brew tap homebrew/science` to the Homebrew installation page'
2013-09-26 11:39:38,365 [   scc.merge] INFO 

@joshmoore
Copy link
Member

Does Candidate PRs == Included PRs ?

@sbesson
Copy link
Member Author

sbesson commented Sep 26, 2013

Yes with --info, this is the set of PRs to be included in the merge. In other terms, Merged PRs + Conflicting PRs == Candidate PRs

@joshmoore
Copy link
Member

Gotcha. Generally looks good then.

This commit should allow the testStatus to pass for any user. Since
the commit is retrieved from the head repo, the authenticated user should
have permissions to set the status on the commit.
@sbesson
Copy link
Member Author

sbesson commented Sep 26, 2013

@joshmoore
Copy link
Member

Ran 134 tests in 558.137s

certainly looks good. SCC isn't self-omitting non-checked status PRs, but otherwise ... 😏

Merging.

joshmoore added a commit that referenced this pull request Sep 27, 2013
Add logic to filter PRs based on the last commit status
@joshmoore joshmoore merged commit f00a409 into ome:master Sep 27, 2013
@sbesson sbesson deleted the pr_status branch September 27, 2013 07:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants