[snowpatch] [PATCH] Initial support for testing from multiple base branches

[Russell Currey]

On Fri, 2016-04-01 at 19:32 +1100, Andrew Donnellan wrote:
> Many large projects have a number of different branches, such as
> "master",
> "next" and "stable". A submitted patch may be intended to be applied on
> top
> of any one of these branches, or possibly for multiple branches.
> 
> To allow testing on multiple base branches, rather than specifying a
> single
> base branch, we now specify a list of base branches in the project
> configuration.
> 
> By default, snowpatch will attempt to apply patches on every base branch
> and trigger a Jenkins job for every branch. By setting "test_all_branches
> =
> false" in the project configuration, snowpatch will go through the listed
> base branches in order and trigger a Jenkins job on the first branch on
> which the patch is successfully applied.
> 
> This support needs some cleanup and some more work, but this is a start.
> 
> Signed-off-by: Andrew Donnellan <andrew.donnellan at au1.ibm.com>

Thanks for our first patch!

> ---
>  examples/openpower.toml |   6 ++-
>  src/main.rs             | 120 +++++++++++++++++++++++++-----------------
> ------
>  src/settings.rs         |   3 +-
>  3 files changed, 68 insertions(+), 61 deletions(-)
> 
> diff --git a/examples/openpower.toml b/examples/openpower.toml
> index 18db146..28671a4 100644
> --- a/examples/openpower.toml
> +++ b/examples/openpower.toml
> @@ -31,14 +31,16 @@ port = 443
>      # the name of the project must be as is in patchwork
>      [projects.skiboot]
>      repository = "/home/ruscur/Documents/skiboot"
> -    branch = "master" # the branch to base from
> +    branches = ["master", "stable"] # branches to base from
> +    test_all_branches = false
>      remote_name = "github"
>      remote_uri = "git at github.com:ruscur/skiboot.git"
>      push_results = false
>  
>      [projects.linuxppc-dev]
>      repository = "/home/ruscur/Documents/linux"
> -    branch = "master" # TODO: multiple merge branches
> +    branches = ["master", "powerpc-next"]
> +    # test_all_branches defaults to true
>      remote_name = "github"
>      remote_uri = "git at github.com:ruscur/linux.git"
>      push_results = false
> diff --git a/src/main.rs b/src/main.rs
> index 8077305..e21ed8c 100644
> --- a/src/main.rs
> +++ b/src/main.rs
> @@ -144,61 +144,71 @@ fn test_patch(settings: &Config, project: &Project,
> path: &Path) -> Vec<TestResu
>      let mut push_opts = PushOptions::new();
>      push_opts.remote_callbacks(push_callbacks);
>  
> -    // Make sure we're up to date
> -    git::pull(&repo).unwrap();
> -
> -    println!("Creating a new branch...");
> -    let mut branch = repo.branch(&tag, &commit, true).unwrap();
> -    println!("Switching to branch...");
> -    repo.set_head(branch.get().name().unwrap())
> -        .unwrap_or_else(|err| panic!("Couldn't set HEAD: {}", err));
> -    repo.checkout_head(None)
> -        .unwrap_or_else(|err| panic!("Couldn't checkout HEAD: {}",
> err));
> -    println!("Repo is now at head {}",
> repo.head().unwrap().name().unwrap());
> -
> -    let output = git::apply_patch(&repo, &path);
> -
> -    match output {
> -        Ok(_) => git::push_to_remote(
> -            &mut remote, &tag, &mut push_opts).unwrap(),
> -        _ => {}
> -    }
> +    for branch_name in project.branches.clone() {
> +        let tag = tag.clone();

We need to have a different tag per branch.  Otherwise, branches are going
to be overridden for everything we try and merge the patch into.  Now this
shouldn't be an issue from what I see, since we wait for the test to finish
before starting the next one (right?). However, we're not deleting remote
branches meaning having a remote history of the stuff we test is good, and
in future I would like it if we kicked off a job for a branch and didn't
block on it completing, and this would be required for that (since the
remote branches would conflict).

Appending the branch on the end should be enough, whether we only want to
do this if test_all_branches is set is up to you.

> +        println!("Switching to base branch {}...", branch_name);
> +        git::checkout_branch(&repo, &branch_name);
> +
> +        // Make sure we're up to date
> +        git::pull(&repo).unwrap();
> +
> +        println!("Creating a new branch...");
> +        let mut branch = repo.branch(&tag, &commit, true).unwrap();
> +        println!("Switching to branch...");
> +        repo.set_head(branch.get().name().unwrap())
> +            .unwrap_or_else(|err| panic!("Couldn't set HEAD: {}", err));
> +        repo.checkout_head(None)
> +            .unwrap_or_else(|err| panic!("Couldn't checkout HEAD: {}",
> err));
> +        println!("Repo is now at head {}",
> repo.head().unwrap().name().unwrap());
> +
> +        let output = git::apply_patch(&repo, &path);
> +
> +        match output {
> +            Ok(_) => git::push_to_remote(
> +                &mut remote, &tag, &mut push_opts).unwrap(),
> +            _ => {}
> +        }
>  
> -    git::checkout_branch(&repo, &project.branch);
> -    // we need to find the branch again since its head has moved
> -    branch = repo.find_branch(&tag, BranchType::Local).unwrap();
> -    branch.delete().unwrap();
> -    println!("Repo is back to {}",
> repo.head().unwrap().name().unwrap());
> -
> -    match output {
> -        Ok(_) => {
> -            results.push(TestResult {
> -                test_name: "apply_patch".to_string(),
> -                state: TestState::SUCCESS.string(),
> -                url: None,
> -                summary: Some("Successfully applied".to_string()),
> -            });
> -        },
> -        Err(_) => {
> -            // It didn't apply.  No need to bother testing.
> -            results.push(TestResult {
> -                test_name: "apply_patch".to_string(),
> -                state: TestState::FAILURE.string(),
> -                url: None,
> -                summary: Some("Series failed to apply to
> branch".to_string()),
> -            });
> -            return results;
> +        git::checkout_branch(&repo, &branch_name);
> +        // we need to find the branch again since its head has moved
> +        branch = repo.find_branch(&tag, BranchType::Local).unwrap();
> +        branch.delete().unwrap();
> +        println!("Repo is back to {}",
> repo.head().unwrap().name().unwrap());
> +
> +        match output {
> +            Ok(_) => {
> +                results.push(TestResult {
> +                    test_name: "apply_patch".to_string(),
> +                    state: TestState::SUCCESS.string(),
> +                    url: None,
> +                    summary: Some("Successfully applied".to_string()),
> +                });
> +            },
> +            Err(_) => {
> +                // It didn't apply.  No need to bother testing.
> +                results.push(TestResult {
> +                    test_name: "apply_patch".to_string(),
> +                    state: TestState::FAILURE.string(),

If we're supporting multiple branches, then failing to apply shouldn't be a
FAILURE, but a WARNING.  Lots of patches aren't going to apply to every
branch.  Maybe set to FAILURE if test_all_branches is false?

Another issue is what happens if test_all_branches is true and it fails to
apply to every branch.  Maybe we can do a check at the end such that if
everything in the Vec<TestResult> is WARNING (i.e. no successes to mark as
success, and no failures to mark as failure), we can add a new one with
FAILURE to make it very clear the patch sucks.

> +                    url: None,
> +                    summary: Some("Series failed to apply to
> branch".to_string()),
> +                });
> +                continue;
> +            }
>          }
> -    }
>  
> -    let settings = settings.clone();
> -    let project = project.clone();
> -    let settings_clone = settings.clone();
> -    // We've set up a remote branch, time to kick off tests
> -    let test = thread::Builder::new().name(tag.to_string()).spawn(move
> || {
> -        return run_tests(&settings_clone, &project, &tag);
> -    }).unwrap();
> -    results.append(&mut test.join().unwrap());
> +        let settings = settings.clone();
> +        let project = project.clone();
> +        let settings_clone = settings.clone();
> +        let test_all_branches =
> project.test_all_branches.unwrap_or(true);
> +
> +        // We've set up a remote branch, time to kick off tests
> +        let test =
> thread::Builder::new().name(tag.to_string()).spawn(move || {
> +            return run_tests(&settings_clone, &project, &tag);
> +        }).unwrap();
> +        results.append(&mut test.join().unwrap());
> +
> +        if test_all_branches { break; }
> +    }
>      results
>  }
>  
> @@ -211,12 +221,6 @@ fn main() {
>      // The HTTP client we'll use to access the APIs
>      let client = Arc::new(Client::new());
>  
> -    // Make sure each project repository is starting at the base branch
> -    for (_, config) in settings.projects.iter() {
> -        let repo = config.get_repo().unwrap();
> -        git::checkout_branch(&repo, &config.branch);
> -    }
> -
>      let mut patchwork = PatchworkServer::new(&settings.patchwork.url,
> &client);
>      if settings.patchwork.user.is_some() {
>          patchwork.set_authentication(&settings.patchwork.user.clone().un
> wrap(),
> diff --git a/src/settings.rs b/src/settings.rs
> index 66063a4..13c4c51 100644
> --- a/src/settings.rs
> +++ b/src/settings.rs
> @@ -45,7 +45,8 @@ pub struct Jenkins {
>  #[derive(RustcDecodable, Clone)]
>  pub struct Project {
>      pub repository: String,
> -    pub branch: String,
> +    pub branches: Vec<String>,
> +    pub test_all_branches: Option<bool>,
>      pub remote_name: String,
>      pub remote_uri: String,
>      pub jobs: Vec<BTreeMap<String, String>>,