[snowpatch] [PATCH 2/8] Require project argument to be specified for daemon mode
Russell Currey
ruscur at russell.cc
Fri Jul 20 21:57:22 AEST 2018
So this might seem out of place, but bear with me here.
Right now, the way that we check for patches is to get the latest
patches "page" from the API. For each of them, we see if the project
the patch is from matches one we have settings for, and if so we
test it. We exhaust the patch list page we got, and then we get a
new one.
There are lots of problems with this. The first one is that if we're
testing lots of patches and the Patchwork instance is high traffic,
we risk missing patches that fall to page 2 or beyond in that time.
We currently don't have a solution for this.
The next issue is that when I originally implemented this, filtering
by project didn't work properly in the patches API, making it
difficult to track all patches, especially for niche projects with
less activity (since running into one patch of a series will lead
to them all being tested, even if snowpatch doesn't find them all
individually in the patch list).
This is no longer the case however, and we can filter by project
on the patch list and actually get a full page of patches in that
project. As a result, until both the following conditions are true:
- patchwork has a suitable events API and/or better pagination
- snowpatch has a proper threading model for distributed work
Limit snowpatch to one project at a time to minimise the chance of
missing patches, and users should just spin up more than one
instance if they want to test multiple projects at once.
Signed-off-by: Russell Currey <ruscur at russell.cc>
---
src/main.rs | 130 +++++++++++++++++++++--------------------------
src/patchwork.rs | 8 ++-
2 files changed, 64 insertions(+), 74 deletions(-)
diff --git a/src/main.rs b/src/main.rs
index 44bc013..5bf6187 100644
--- a/src/main.rs
+++ b/src/main.rs
@@ -67,8 +67,8 @@ mod utils;
static USAGE: &'static str = "
Usage:
- snowpatch <config-file> [--count=<count>] [--project <name>]
- snowpatch <config-file> --mbox <mbox> --project <name>
+ snowpatch <config-file> --project <name> [--count=<count>]
+ snowpatch <config-file> --project <name> --mbox <mbox>
snowpatch <config-file> --patch <id>
snowpatch <config-file> --series <id>
snowpatch -v | --version
@@ -77,11 +77,11 @@ Usage:
By default, snowpatch runs as a long-running daemon.
Options:
+ --project <name> Test patches for the given project.
--count <count> Run tests on <count> recent series.
--patch <id> Run tests on the given Patchwork patch.
--series <id> Run tests on the given Patchwork series.
--mbox <mbox> Run tests on the given mbox file. Requires --project
- --project <name> Test patches for the given project.
-v, --version Output version information.
-h, --help Output this help text.
";
@@ -349,19 +349,6 @@ fn main() {
panic!("Can't specify both --series and --patch");
}
- if args.flag_mbox != "" && args.flag_project != "" {
- info!("snowpatch is testing a local patch.");
- let patch = Path::new(&args.flag_mbox);
- match settings.projects.get(&args.flag_project) {
- None => panic!("Couldn't find project {}", args.flag_project),
- Some(project) => {
- test_patch(&settings, &client, project, patch, true);
- }
- }
-
- return;
- }
-
if args.flag_patch > 0 {
info!("snowpatch is testing a patch from Patchwork.");
let patch = patchwork.get_patch(&(args.flag_patch as u64)).unwrap();
@@ -400,6 +387,17 @@ fn main() {
return;
}
+ // At this point, specifying a project is required
+ let project = settings.projects.get(&args.flag_project).unwrap();
+
+ if args.flag_mbox != "" {
+ info!("snowpatch is testing a local patch.");
+ let patch = Path::new(&args.flag_mbox);
+ test_patch(&settings, &client, project, patch, true);
+
+ return;
+ }
+
// The number of patches tested so far. If --count isn't provided, this is unused.
let mut patch_count = 0;
@@ -411,7 +409,7 @@ fn main() {
*/
'daemon: loop {
let patch_list = patchwork
- .get_patch_query()
+ .get_patch_query(&args.flag_project)
.unwrap_or_else(|err| panic!("Failed to obtain patch list: {}", err));
info!("snowpatch is ready to test new revisions from Patchwork.");
for patch in patch_list {
@@ -426,72 +424,60 @@ fn main() {
continue;
}
- //let project = patchwork.get_project(&patch.project).unwrap();
- // Skip if we're using -p and it's the wrong project
- if args.flag_project != "" && patch.project.link_name != args.flag_project {
- debug!(
+ // Skip if it's the wrong project
+ if patch.project.link_name != args.flag_project {
+ warn!(
"Skipping patch {} ({}) (wrong project: {})",
patch.name, patch.id, patch.project.link_name
);
continue;
}
- match settings.projects.get(&patch.project.link_name) {
- None => {
- debug!(
- "Project {} not configured for patch {}",
- &patch.project.link_name, patch.name
- );
- continue;
- }
- Some(project) => {
- // TODO(ajd): Refactor this.
- let hefty_tests;
- let mbox = if patch.has_series() {
- debug!(
- "Patch {} has a series at {}!",
- &patch.name, &patch.series[0].url
- );
- let series = patchwork.get_series_by_url(&patch.series[0].url);
- match series {
- Ok(series) => {
- if !series.received_all {
- debug!("Series is incomplete, skipping patch for now");
- continue;
- }
- let dependencies = patchwork.get_patch_dependencies(&patch);
- hefty_tests = dependencies.len() == series.patches.len();
- patchwork.get_patches_mbox(dependencies)
- }
- Err(e) => {
- debug!("Series is not OK: {}", e);
- hefty_tests = true;
- patchwork.get_patch_mbox(&patch)
- }
+ // TODO(ajd): Refactor this.
+ let hefty_tests;
+ let mbox = if patch.has_series() {
+ debug!(
+ "Patch {} has a series at {}!",
+ &patch.name, &patch.series[0].url
+ );
+ let series = patchwork.get_series_by_url(&patch.series[0].url);
+ match series {
+ Ok(series) => {
+ if !series.received_all {
+ debug!("Series is incomplete, skipping patch for now");
+ continue;
}
- } else {
+ let dependencies = patchwork.get_patch_dependencies(&patch);
+ hefty_tests = dependencies.len() == series.patches.len();
+ patchwork.get_patches_mbox(dependencies)
+ }
+ Err(e) => {
+ debug!("Series is not OK: {}", e);
hefty_tests = true;
patchwork.get_patch_mbox(&patch)
- };
-
- let results = test_patch(&settings, &client, project, &mbox, hefty_tests);
-
- // Delete the temporary directory with the patch in it
- fs::remove_dir_all(mbox.parent().unwrap())
- .unwrap_or_else(|err| error!("Couldn't delete temp directory: {}", err));
- if project.push_results {
- for result in results {
- patchwork.post_test_result(result, &patch.checks).unwrap();
- }
- }
- if args.flag_count > 0 {
- patch_count += 1;
- debug!("Tested {} patches out of {}", patch_count, args.flag_count);
- if patch_count >= args.flag_count {
- break 'daemon;
- }
}
}
+ } else {
+ hefty_tests = true;
+ patchwork.get_patch_mbox(&patch)
+ };
+
+ let results = test_patch(&settings, &client, project, &mbox, hefty_tests);
+
+ // Delete the temporary directory with the patch in it
+ fs::remove_dir_all(mbox.parent().unwrap())
+ .unwrap_or_else(|err| error!("Couldn't delete temp directory: {}", err));
+ if project.push_results {
+ for result in results {
+ patchwork.post_test_result(result, &patch.checks).unwrap();
+ }
+ }
+ if args.flag_count > 0 {
+ patch_count += 1;
+ debug!("Tested {} patches out of {}", patch_count, args.flag_count);
+ if patch_count >= args.flag_count {
+ break 'daemon;
+ }
}
}
info!("Finished testing new revisions, sleeping.");
diff --git a/src/patchwork.rs b/src/patchwork.rs
index d59fc40..378c6ca 100644
--- a/src/patchwork.rs
+++ b/src/patchwork.rs
@@ -300,8 +300,12 @@ impl PatchworkServer {
serde_json::from_str(&self.get_url_string(url).unwrap())
}
- pub fn get_patch_query(&self) -> Result<Vec<Patch>, serde_json::Error> {
- let url = format!("{}{}/patches/{}", &self.url, PATCHWORK_API, PATCHWORK_QUERY);
+ pub fn get_patch_query(&self, project: &str) -> Result<Vec<Patch>, serde_json::Error> {
+ let url = format!(
+ "{}{}/patches/{}&project={}",
+ &self.url, PATCHWORK_API, PATCHWORK_QUERY, project
+ );
+
serde_json::from_str(&self.get_url_string(&url)
.unwrap_or_else(|err| panic!("Failed to connect to Patchwork: {}", err)))
}
--
2.17.1
More information about the snowpatch
mailing list