From 1c57530e2fb8a319c64c353f468e0be2d712b05a Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Fri, 8 Sep 2017 17:56:00 -0400 Subject: [PATCH 1/8] handlers/commands: remove unnecessary argument --- src/handlers/common/handlers/commands.rs | 6 +++--- src/handlers/common/handlers/merge_requests.rs | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs index 640880d8..1c634a8e 100644 --- a/src/handlers/common/handlers/commands.rs +++ b/src/handlers/common/handlers/commands.rs @@ -179,7 +179,7 @@ impl<'a> Commands<'a> { } /// Error messages from the commands. - pub fn error_messages(&self, author: &User, branch: &Branch) -> Vec<String> { + pub fn error_messages(&self, author: &User) -> Vec<String> { let mut messages = vec![]; if !self.unrecognized.is_empty() { @@ -192,7 +192,7 @@ impl<'a> Commands<'a> { if !unavailable.is_empty() { messages.push(format!("@{}: the following commands are not supported for `{}`: `{}`.", author.handle, - branch.name, + self.branch.name, unavailable.join("`, `"))); } @@ -200,7 +200,7 @@ impl<'a> Commands<'a> { if !disallowed.is_empty() { messages.push(format!("@{}: insufficient privileges for the commands for `{}`: `{}`.", author.handle, - branch.name, + self.branch.name, disallowed.join("`, `"))); } diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index d4f9505e..42eba072 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -432,7 +432,7 @@ pub fn handle_merge_request_note(data: &Value, host: &Host, mr_note: MergeReques mr.merge_request.url)))); } - let errors = commands.error_messages(¬e.author, branch); + let errors = commands.error_messages(¬e.author); let defer = if errors.is_empty() { handle_commands(data, host, -- GitLab From 0f5815c99df9555332b6e3c78037e8bc29155a39 Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Fri, 8 Sep 2017 18:27:29 -0400 Subject: [PATCH 2/8] nits: use Vec::new() rather than vec![] --- src/config/types.rs | 4 ++-- src/handlers/common/handlers/commands.rs | 8 ++++---- src/handlers/common/handlers/merge_requests.rs | 6 +++--- src/handlers/common/handlers/push.rs | 2 +- src/main.rs | 2 +- 5 files changed, 11 insertions(+), 11 deletions(-) diff --git a/src/config/types.rs b/src/config/types.rs index a127865c..85c0fb04 100644 --- a/src/config/types.rs +++ b/src/config/types.rs @@ -412,13 +412,13 @@ impl merge::MergePolicy for WorkflowMergePolicy { type Filter = WorkflowMergePolicyFilter; fn for_mr(&self, _: &MergeRequest) -> Self::Filter { - let mut trailers = vec![]; + let mut trailers = Vec::new(); trailers.push(Trailer::new("Acked-by", format!("{}", self.identity))); WorkflowMergePolicyFilter { trailers: trailers, - errors: vec![], + errors: Vec::new(), } } } diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs index 1c634a8e..8e3822d9 100644 --- a/src/handlers/common/handlers/commands.rs +++ b/src/handlers/common/handlers/commands.rs @@ -139,7 +139,7 @@ macro_rules! check_actions { macro_rules! check_action_ok { { $method:ident, $( $action:expr => $name:expr, )* } => { - let mut results = vec![]; + let mut results = Vec::new(); $( if $action.$method() { results.push($name); @@ -151,7 +151,7 @@ macro_rules! check_action_ok { macro_rules! check_action_consistency { { $( ($action1:expr, $action2:expr) => ($name1:expr, $name2:expr), )* } => { - let mut results = vec![]; + let mut results = Vec::new(); $( if $action1.requested() && $action2.requested() { results.push(concat!($name1, "` and `", $name2)); @@ -174,13 +174,13 @@ impl<'a> Commands<'a> { test: CommandState::Unrequested, reformat: CommandState::Unrequested, - unrecognized: vec![], + unrecognized: Vec::new(), } } /// Error messages from the commands. pub fn error_messages(&self, author: &User) -> Vec<String> { - let mut messages = vec![]; + let mut messages = Vec::new(); if !self.unrecognized.is_empty() { messages.push(format!("@{}: the following commands are not recognized at all: `{}`.", diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index 42eba072..f6eaa3ba 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -35,7 +35,7 @@ pub fn handle_merge_request_update(data: &Value, host: &Host, mr: MergeRequestIn &mr.merge_request.target_repo.name, &mr.merge_request.target_branch)); - let mut comment_notes = vec![]; + let mut comment_notes = Vec::new(); if let Some(test_action) = branch.test() { handle_test_on_update(data, test_action, &mr); @@ -423,7 +423,7 @@ pub fn handle_merge_request_note(data: &Value, host: &Host, mr_note: MergeReques return Ok(HandlerResult::Reject("no commands present".to_string())); } - let mut comment_notes = vec![]; + let mut comment_notes = Vec::new(); let fetch_res = host.service.fetch_mr(&project.context, &mr.merge_request); if let Err(err) = fetch_res { @@ -668,7 +668,7 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: & ("arguments".to_string(), args), ("merge_request".to_string(), data.data.clone()) ]))); - (vec![], + (Vec::new(), Some(test_jobs.test_mr(&mr.merge_request, job_data) .map_err(Into::into))) }, diff --git a/src/handlers/common/handlers/push.rs b/src/handlers/common/handlers/push.rs index 1b6df98d..6c641272 100644 --- a/src/handlers/common/handlers/push.rs +++ b/src/handlers/common/handlers/push.rs @@ -79,7 +79,7 @@ pub fn handle_push(data: &Value, host: &Host, push: PushInfo) -> JobResult<Handl } } - let mut comment_notes = vec![]; + let mut comment_notes = Vec::new(); let fetch_res = project.context.force_fetch_into("origin", refname, refname); if let Err(err) = fetch_res { diff --git a/src/main.rs b/src/main.rs index 6f8c864e..1e3233dc 100644 --- a/src/main.rs +++ b/src/main.rs @@ -75,7 +75,7 @@ fn run_director(config_path: &Path) -> Result<RunResult> { let mut config = Config::from_path(&config_path, handlers::connect_to_host)?; let watchdog = DirectorWatchdog {}; - let mut handlers = vec![]; + let mut handlers = Vec::new(); for (name, host) in config.hosts.drain() { handlers.push(handlers::create_handler(host, &name)?) -- GitLab From 5b742639419a15eb3fc56331fe0b624d10941b72 Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Fri, 8 Sep 2017 18:27:46 -0400 Subject: [PATCH 3/8] gitlab: simplify the token extraction --- src/handlers/gitlab/handler.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/src/handlers/gitlab/handler.rs b/src/handlers/gitlab/handler.rs index ae6a9358..8553c228 100644 --- a/src/handlers/gitlab/handler.rs +++ b/src/handlers/gitlab/handler.rs @@ -27,9 +27,10 @@ use std::sync::Arc; /// Connect to a Gitlab from its configuration block. pub fn connect_to_host(url: &Option<String>, secrets: Value) -> Result<Arc<HostingService>> { let host = url.as_ref().map_or("gitlab.com", |u| u.as_str()); - let token = match secrets.pointer("/token").and_then(|t| t.as_str()) { - Some(t) => t, - None => return Err("gitlab requires a token to be provided".into()), + let token = if let Some(token) = secrets.pointer("/token").and_then(|t| t.as_str()) { + token + } else { + bail!("gitlab requires a token to be provided"); }; let use_ssl = secrets.pointer("/use_ssl").and_then(Value::as_bool).unwrap_or(true); -- GitLab From 273f355d0358582430db94f6334a155f7fbcc38e Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Fri, 8 Sep 2017 18:24:00 -0400 Subject: [PATCH 4/8] commands: make unrecognized private There's no need to have it be public. --- src/handlers/common/handlers/commands.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/common/handlers/commands.rs b/src/handlers/common/handlers/commands.rs index 8e3822d9..9ea06584 100644 --- a/src/handlers/common/handlers/commands.rs +++ b/src/handlers/common/handlers/commands.rs @@ -99,7 +99,7 @@ pub struct Commands<'a> { pub reformat: CommandState, /// List of unrecognized commands. - pub unrecognized: Vec<String>, + unrecognized: Vec<String>, } macro_rules! check_action { -- GitLab From a261aca0fb13c3de71e4742d1c3ed5002daf7876 Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Sat, 9 Sep 2017 22:34:38 -0400 Subject: [PATCH 5/8] merge_requests: use the json! macro --- src/handlers/common/handlers/merge_requests.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index f6eaa3ba..b8134262 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -17,7 +17,7 @@ use crates::git_workarea::{CommitId, GitContext}; use crates::itertools::Itertools; use crates::json_job_dispatch::HandlerResult; use crates::json_job_dispatch::Result as JobResult; -use crates::serde_json::{Map, Value}; +use crates::serde_json::Value; use config::{Branch, Host, CheckAction, MergeAction, Project, ReformatAction, StageAction, StageUpdatePolicy, TestAction, TestBackend}; @@ -26,7 +26,7 @@ use handlers::common::handlers::commands::Commands; use handlers::common::handlers::error::Error; use handlers::common::handlers::utils; -use std::iter::{self, FromIterator}; +use std::iter; /// Handle an update to a merge request. pub fn handle_merge_request_update(data: &Value, host: &Host, mr: MergeRequestInfo) @@ -664,10 +664,10 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: & .map(|arg| Value::String(arg.clone())) .collect()); let job_data = utils::test_job("test_action", - &Value::Object(Map::from_iter(vec![ - ("arguments".to_string(), args), - ("merge_request".to_string(), data.data.clone()) - ]))); + &json!({ + "arguments": args, + "merge_request": data.data.clone(), + })); (Vec::new(), Some(test_jobs.test_mr(&mr.merge_request, job_data) .map_err(Into::into))) -- GitLab From 2ede23cc43ac2a8afcec35cea39796b8db4e9f7a Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Sat, 9 Sep 2017 22:34:54 -0400 Subject: [PATCH 6/8] merge_requests: fix a typo in an error message --- src/handlers/common/handlers/merge_requests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index b8134262..32203823 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -338,7 +338,7 @@ fn check_mr(project: &Project, branch: &Branch, mr: &MergeRequestInfo) -> check: Ok(commit) => commit, Err(err) => { error!(target: "ghostflow-director/handler", - "failed to determin ethe commit to backport into {}: {:?}", + "failed to determine the commit to backport into {}: {:?}", backport.branch(), err); -- GitLab From bc28dbe1fbbd26d9e221c2df306f3b07d225086c Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Sat, 9 Sep 2017 22:35:17 -0400 Subject: [PATCH 7/8] merge_requests: mention that the backport is what's wrong --- src/handlers/common/handlers/merge_requests.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index 32203823..b1a8ec3d 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -356,7 +356,7 @@ fn check_mr(project: &Project, branch: &Branch, mr: &MergeRequestInfo) -> check: .map(|results| results.into_iter().all(|status| status == check::CheckStatus::Pass)); if !comment_notes.is_empty() { - let comment = format!("The branch configuration is not valid:\n\ + let comment = format!("The backport configuration is not valid:\n\ \n - {}", comment_notes.into_iter().join("\n - ")); if let Err(err) = project.service.post_mr_comment(&mr.merge_request, &comment) { -- GitLab From ddf5cb38f10bb5321a4a99a6ba92467bdcf639f1 Mon Sep 17 00:00:00 2001 From: Ben Boeckel <ben.boeckel@kitware.com> Date: Sat, 9 Sep 2017 22:35:39 -0400 Subject: [PATCH 8/8] merge_requests: use clap for `Do: test` argument parsing --- .../common/handlers/merge_requests.rs | 68 ++++++++----------- 1 file changed, 27 insertions(+), 41 deletions(-) diff --git a/src/handlers/common/handlers/merge_requests.rs b/src/handlers/common/handlers/merge_requests.rs index b1a8ec3d..d5f48ce7 100644 --- a/src/handlers/common/handlers/merge_requests.rs +++ b/src/handlers/common/handlers/merge_requests.rs @@ -658,7 +658,7 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: & let mr = &data.mr_note.merge_request; let test = action.test(); - let (arguments, res): (Vec<String>, Option<test::Result<_>>) = match *test { + let res: test::Result<_> = match *test { TestBackend::Jobs(ref test_jobs) => { let args = Value::Array(arguments.iter() .map(|arg| Value::String(arg.clone())) @@ -668,54 +668,40 @@ fn handle_test_command(data: &mut CommandData, action: &TestAction, arguments: & "arguments": args, "merge_request": data.data.clone(), })); - (Vec::new(), - Some(test_jobs.test_mr(&mr.merge_request, job_data) - .map_err(Into::into))) + test_jobs.test_mr(&mr.merge_request, job_data) + .map_err(Into::into) }, TestBackend::Refs(ref test_refs) => { - let mut stop = false; - - let arguments = arguments.iter() - .filter_map(|arg| { - if arg == "--stop" { - stop = true; - None + let matches = command_app("test-action") + .arg(Arg::with_name("STOP") + .long("stop") + .takes_value(false)) + .get_matches_from_safe(arguments); + + match matches { + Ok(matches) => { + let res = if matches.is_present("STOP") { + test_refs.untest_mr(&mr.merge_request) } else { - Some(arg.to_string()) - } - }) - .collect::<Vec<_>>(); + test_refs.test_mr(&mr.merge_request) + }; - let res = if arguments.is_empty() { - let res = if stop { - test_refs.untest_mr(&mr.merge_request) - } else { - test_refs.test_mr(&mr.merge_request) - }; - - Some(res.map_err(Into::into)) - } else { - None - }; + res.map_err(Into::into) + }, + Err(err) => { + data.notes.push(format!("Unrecognized `merge` arguments: `{}`.", + err.message.lines().next().unwrap())); - (arguments, res) + Ok(()) + }, + } }, }; - if arguments.is_empty() != res.is_some() { - error!(target: "ghostflow-director/handler", - "a test action occurred even though some arguments were unparsed"); - } - - if arguments.is_empty() { - if let Some(Err(err)) = res { - data.notes.push(format!("Error occurred during test action ({}): `{:?}`", - data.host.maintainers.join(" "), - err)); - } - } else { - data.notes.push(format!("Unrecognized `test` arguments: `{}`.", - arguments.iter().join("`, `"))); + if let Err(err) = res { + data.notes.push(format!("Error occurred during test action ({}): `{:?}`", + data.host.maintainers.join(" "), + err)); } CommandResult::Continue -- GitLab