Implement review processing code #5
Conversation
WalkthroughThe changes primarily focus on enhancing the functionality of the Changes
TipsChat with CodeRabbit Bot (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review Status
Actionable comments generated: 33
Files selected for processing (13)
- vibi-dpu/src/bitbucket/auth.rs (1 hunks)
- vibi-dpu/src/bitbucket/user.rs (2 hunks)
- vibi-dpu/src/core/mod.rs (1 hunks)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/db/hunk.rs (1 hunks)
- vibi-dpu/src/db/mod.rs (1 hunks)
- vibi-dpu/src/db/review.rs (1 hunks)
- vibi-dpu/src/pubsub/listener.rs (2 hunks)
- vibi-dpu/src/utils/gitops.rs (1 hunks)
- vibi-dpu/src/utils/hunk.rs (1 hunks)
- vibi-dpu/src/utils/lineitem.rs (1 hunks)
- vibi-dpu/src/utils/mod.rs (1 hunks)
- vibi-dpu/src/utils/review.rs (1 hunks)
Files skipped from review due to trivial changes (4)
- vibi-dpu/src/core/mod.rs
- vibi-dpu/src/db/mod.rs
- vibi-dpu/src/utils/lineitem.rs
- vibi-dpu/src/utils/mod.rs
Additional comments (Suppressed): 2
vibi-dpu/src/utils/review.rs (1)
- 87-126: Setter methods are provided for all fields of the
Reviewstruct. This could potentially lead to misuse where an object's state is changed unexpectedly. If some fields are not meant to be changed after the object is created, consider removing their setter methods.vibi-dpu/src/pubsub/listener.rs (1)
- 15-15: The import statement for
process_reviewis commented out in the old hunk but is active in the new hunk. Ensure that the functionprocess_reviewis implemented and available in thereviewmodule before this change is merged.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 67
Files ignored due to filter (1)
- vibi-dpu/Cargo.toml
Files selected for processing (15)
- vibi-dpu/src/bitbucket/auth.rs (1 hunks)
- vibi-dpu/src/bitbucket/config.rs (1 hunks)
- vibi-dpu/src/bitbucket/mod.rs (1 hunks)
- vibi-dpu/src/bitbucket/user.rs (1 hunks)
- vibi-dpu/src/bitbucket/webhook.rs (1 hunks)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/core/setup.rs (1 hunks)
- vibi-dpu/src/db/auth.rs (1 hunks)
- vibi-dpu/src/db/hunk.rs (1 hunks)
- vibi-dpu/src/db/owner.rs (1 hunks)
- vibi-dpu/src/db/review.rs (1 hunks)
- vibi-dpu/src/db/user.rs (1 hunks)
- vibi-dpu/src/db/webhook.rs (1 hunks)
- vibi-dpu/src/utils/gitops.rs (1 hunks)
- vibi-dpu/src/utils/hunk.rs (1 hunks)
Files skipped from review due to trivial changes (2)
- vibi-dpu/src/bitbucket/mod.rs
- vibi-dpu/src/utils/hunk.rs
Additional comments (Suppressed): 2
vibi-dpu/src/db/owner.rs (1)
- 14-14: The key for the database entry is being formatted as "owners:{uuid}". Ensure that this format is consistent with other parts of the application where this key might be used or read from.
vibi-dpu/src/db/webhook.rs (1)
- 4-5: The
get_dbfunction andWebhookstruct are imported but there is no context provided about their implementation. Ensure that these components are correctly implemented and tested.
There was a problem hiding this comment.
Review Status
Actionable comments generated: 68
Files selected for processing (17)
- vibi-dpu/src/bitbucket/auth.rs (1 hunks)
- vibi-dpu/src/bitbucket/config.rs (1 hunks)
- vibi-dpu/src/bitbucket/repo.rs (1 hunks)
- vibi-dpu/src/bitbucket/user.rs (1 hunks)
- vibi-dpu/src/bitbucket/webhook.rs (1 hunks)
- vibi-dpu/src/bitbucket/workspace.rs (1 hunks)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/core/setup.rs (1 hunks)
- vibi-dpu/src/db/auth.rs (1 hunks)
- vibi-dpu/src/db/hunk.rs (1 hunks)
- vibi-dpu/src/db/owner.rs (1 hunks)
- vibi-dpu/src/db/repo.rs (1 hunks)
- vibi-dpu/src/db/review.rs (1 hunks)
- vibi-dpu/src/db/user.rs (1 hunks)
- vibi-dpu/src/db/webhook.rs (1 hunks)
- vibi-dpu/src/utils/gitops.rs (1 hunks)
- vibi-dpu/src/utils/repo.rs (1 hunks)
Files skipped from review due to trivial changes (1)
- vibi-dpu/src/db/repo.rs
Additional comments (Suppressed): 3
vibi-dpu/src/utils/repo.rs (3)
1-15: The
Repositorystruct is well defined with all necessary fields. It's good to see that you've implemented theClone,Serialize, andDeserializetraits which will be useful for cloning instances, and serializing/deserializing the struct.19-41: The constructor function
newis correctly implemented and takes in all the necessary parameters to create a new instance ofRepository.44-78: The getter methods are correctly implemented and return references to the struct fields. This is a good practice as it allows read access to the fields without giving the ability to modify them directly.
vibi-dpu/src/bitbucket/webhook.rs
Outdated
| async fn process_add_webhook_response(response: Result<Response, Error>){ | ||
| if response.is_err() { | ||
| let err = response.expect_err("No error in response"); | ||
| eprintln!("Error in api call: {:?}", err); | ||
| return; | ||
| } | ||
| let res = response.expect("Uncaught error in response"); | ||
| if !res.status().is_success() { | ||
| eprintln!("Failed to add webhook. Status code: {}, Text: {:?}", | ||
| res.status(), res.text().await); | ||
| return; | ||
| } | ||
| let webhook_res = res.json::<WebhookResponse>().await; | ||
| if webhook_res.is_err() { | ||
| let err = webhook_res.expect_err("No error in webhook response"); | ||
| eprintln!("Failed to parse webhook_res: {:?}", err); | ||
| return; | ||
| } | ||
| let webhook = webhook_res.expect("Uncaught error in webhook response"); | ||
| let webhook_data = Webhook::new( | ||
| webhook.uuid().to_string(), | ||
| webhook.active(), | ||
| webhook.created_at().to_string(), | ||
| webhook.events().to_owned(), | ||
| webhook.links()["self"]["href"].clone(), | ||
| webhook.url().to_string(), | ||
| ); | ||
| save_webhook_to_db(&webhook_data); | ||
| } |
There was a problem hiding this comment.
Similar to the previous comment, the process_add_webhook_response function could benefit from returning a Result type. This way, you can propagate errors up to the caller instead of just printing them and continuing execution. This would allow the caller to decide how to handle these errors.
| let insert_res = db.insert(IVec::from(review_key.as_bytes()), json); | ||
| if insert_res.is_err() { | ||
| let e = insert_res.expect_err("No error in insert_res"); | ||
| eprintln!("Failed to upsert review into sled DB: {e}"); |
There was a problem hiding this comment.
There's a typo in the error print statement. The variable e should be wrapped in curly braces to be correctly interpolated into the string.
- eprintln!("Failed to upsert review into sled DB: {e}");
+ eprintln!("Failed to upsert review into sled DB: {}", e);
vibi-dpu/src/bitbucket/config.rs
Outdated
| async fn get_all_pages(next_url: Option<String>, access_token: &str, params: &Option<HashMap<&str, &str>>) -> Vec<Value>{ | ||
| let mut values_vec = Vec::new(); | ||
| let mut next_url_mut = next_url; | ||
| while next_url_mut.is_some() { | ||
| let url_opt = next_url_mut.as_ref(); | ||
| if url_opt.is_none() { | ||
| eprintln!("next_url is none"); | ||
| break; | ||
| } | ||
| let url = url_opt.expect("Empty next url_opt").trim_matches('"'); | ||
| if url == "null" { | ||
| break; | ||
| } | ||
| let response_opt = get_api(url, access_token, params).await; | ||
| let (mut response_values, url_opt) = deserialize_response(response_opt).await; | ||
| next_url_mut = url_opt.clone(); | ||
| values_vec.append(&mut response_values); | ||
| } | ||
| return values_vec; | ||
| } |
There was a problem hiding this comment.
In the get_all_pages function, you're using expect which will panic if its condition is not met. Instead, consider handling this error gracefully and return an appropriate error message to the caller. This will prevent your application from crashing unexpectedly.
vibi-dpu/src/bitbucket/config.rs
Outdated
| pub fn prepare_auth_headers(access_token: &str) -> Option<HeaderMap>{ | ||
| let mut headers_map = HeaderMap::new(); | ||
| let auth_header = format!("Bearer {}", access_token); | ||
| let headervalres = HeaderValue::from_str(&auth_header); | ||
| if headervalres.is_err() { | ||
| let e = headervalres.expect_err("No error found in headervalres"); | ||
| eprintln!("Could not parse header value: {}", e); | ||
| return None; | ||
| } | ||
| let headerval = headervalres.expect("Empty headervalres"); | ||
| headers_map.insert("Authorization", headerval); | ||
| return Some(headers_map); | ||
| } |
There was a problem hiding this comment.
In the prepare_auth_headers function, you're using expect_err and expect which will panic if their conditions are not met. Instead, consider handling these errors gracefully and return an appropriate error message to the caller. This will prevent your application from crashing unexpectedly.
vibi-dpu/src/utils/gitops.rs
Outdated
| pub fn commit_exists(commit: &str, directory: &str) -> bool { | ||
| let output_res = Command::new("git") | ||
| .arg("rev-list") | ||
| .arg(commit) | ||
| .current_dir(directory) | ||
| .output(); | ||
| if output_res.is_err() { | ||
| let e = output_res.expect_err("No error in output_res"); | ||
| eprintln!("Failed to start git rev-list: {:?}", e); | ||
| return false; | ||
| } | ||
| let output = output_res.expect("Uncaught error in output_res"); | ||
| println!("Execute git rev-list, exit code: {:?}", output.status.code()); | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => println!("git rev-list stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git rev-list stderr error {}", e)}, | ||
| }; | ||
| match str::from_utf8(&output.stdout) { | ||
| Ok(v) => println!("git rev-list stdout = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git rev-list stdout error {}", e)}, | ||
| }; | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The function commit_exists always returns true, regardless of whether the commit exists or not. The function should return false if the command fails or if the commit does not exist in the repository. Consider checking the output of the git command to determine if the commit exists.
vibi-dpu/src/utils/gitops.rs
Outdated
| pub async fn git_pull(review: &Review) { | ||
| let directory = review.clone_dir(); | ||
| println!("directory = {}", &directory); | ||
| let access_token_opt = refresh_git_auth(review.clone_url(), review.clone_dir()).await; | ||
| if access_token_opt.is_none() { | ||
| eprintln!("Unable to get access_token from refresh_git_auth"); | ||
| return; | ||
| } | ||
| let access_token = access_token_opt.expect("Empty access_token"); | ||
| set_git_url(review.clone_url(), directory, &access_token); | ||
| let output_res = Command::new("git") | ||
| .arg("pull") | ||
| .current_dir(directory) | ||
| .output(); | ||
| if output_res.is_err() { | ||
| let e = output_res.expect_err("No error in output_res"); | ||
| eprintln!("failed to execute git pull: {:?}", e); | ||
| return; | ||
| } | ||
| let output = output_res.expect("Uncaught error in output_res"); | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => println!("git pull stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git pull stderr error {}", e)}, | ||
| }; | ||
| match str::from_utf8(&output.stdout) { | ||
| Ok(v) => println!("git pull stdout = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git pull stdout error {}", e)}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
In the git_pull function, if the refresh_git_auth function fails to get an access token, the function simply prints an error message and returns. This could lead to unexpected behavior if the caller expects the repository to be up-to-date after this function is called. Consider propagating the error up to the caller so they can handle it appropriately.
| fn process_diff(filepath: &str, diff: &str, linemap: &mut HashMap<String, Vec<String>> ) -> HashMap<String, Vec<String>> { | ||
| let mut limiterpos = Vec::new(); | ||
| let delimitter = "@@"; | ||
| for (idx, _) in diff.match_indices(delimitter) { | ||
| limiterpos.push(idx); | ||
| } | ||
| let mut idx: usize = 0; | ||
| let len = limiterpos.len(); | ||
| while (idx + 1) < len { | ||
| let line_res = diff.get( | ||
| (limiterpos[idx]+delimitter.len())..limiterpos[idx+1] | ||
| ); | ||
| if line_res.is_none() { | ||
| eprintln!("Unable to format diff line"); | ||
| continue; | ||
| } | ||
| let line = line_res.expect("Empty line_res"); | ||
| let sublines: Vec<&str> = line.split(" ").collect(); | ||
| if line.contains("\n") || sublines.len() != 4 { | ||
| idx += 1; | ||
| continue; | ||
| } | ||
| let mut deletionstr = sublines[1].to_owned(); | ||
| // let additionstr = sublines[1]; | ||
| if deletionstr.contains("-") { | ||
| deletionstr = deletionstr.replace("-", ""); | ||
| if deletionstr.contains(",") { | ||
| let delsplit: Vec<&str> = deletionstr.split(",").collect(); | ||
| let delidx = delsplit[0].parse::<i32>().unwrap(); | ||
| let deldiff = delsplit[1].parse::<i32>().unwrap(); | ||
| deletionstr = format!("{delidx},{}", delidx+deldiff); | ||
| } | ||
| else { | ||
| let delidx = deletionstr.parse::<i32>().unwrap(); | ||
| deletionstr.push_str(format!(",{}", delidx).as_str()); | ||
| } | ||
| } | ||
| else { | ||
| idx += 1; | ||
| continue; | ||
| } | ||
| if linemap.contains_key(filepath) { | ||
| linemap.get_mut(filepath).unwrap().push(deletionstr); | ||
| } | ||
| else { | ||
| linemap.insert(filepath.to_string(), vec!(deletionstr)); | ||
| } | ||
| idx += 1; | ||
| } | ||
| return linemap.to_owned(); | ||
| } |
There was a problem hiding this comment.
The process_diff function assumes that the diff output is in a specific format. If the diff output does not match this format, the function may not work as expected. Consider adding checks to ensure that the diff output is in the expected format before attempting to process it.
vibi-dpu/src/utils/auth.rs
Outdated
| use serde::Deserialize; | ||
| use serde::Serialize; | ||
|
|
||
| #[derive(Debug, Deserialize, Serialize, Clone)] | ||
| pub struct AuthInfo { | ||
| access_token: String, | ||
| refresh_token: String, | ||
| expires_in: u64, | ||
| timestamp: Option<u64>, | ||
| } | ||
|
|
||
| impl AuthInfo { | ||
| // Constructor | ||
| pub fn new(access_token: String, refresh_token: String, expires_in: u64, timestamp: Option<u64>) -> Self { | ||
| Self { | ||
| access_token, | ||
| refresh_token, | ||
| expires_in, | ||
| timestamp, | ||
| } | ||
| } | ||
|
|
||
| // Public getter methods | ||
| pub fn access_token(&self) -> &String { | ||
| &self.access_token | ||
| } | ||
|
|
||
| pub fn refresh_token(&self) -> &String { | ||
| &self.refresh_token | ||
| } | ||
|
|
||
| pub fn expires_in(&self) -> u64 { | ||
| self.expires_in | ||
| } | ||
|
|
||
| pub fn timestamp(&self) -> &Option<u64> { | ||
| &self.timestamp | ||
| } | ||
|
|
||
| // Public setters | ||
| pub fn set_timestamp(&mut self, timestamp: u64) { | ||
| self.timestamp = Some(timestamp); | ||
| } | ||
| } |
There was a problem hiding this comment.
The AuthInfo struct and its methods look good. The constructor, getters, and setters are correctly implemented. However, consider adding a method to check if the access token is expired or not. This could be useful in other parts of your code where you need to check the validity of the token before making API calls.
impl AuthInfo {
// ...
pub fn is_expired(&self) -> bool {
match self.timestamp {
Some(timestamp) => {
let current_time = std::time::SystemTime::now()
.duration_since(std::time::UNIX_EPOCH)
.expect("Time went backwards")
.as_secs();
current_time > timestamp + self.expires_in
}
None => true,
}
}
}
vibi-dpu/src/pubsub/listener.rs
Outdated
| async fn process_message(attributes: &HashMap<String, String>, data_bytes: &Vec<u8>) { | ||
| let msgtype_opt = attributes.get("msgtype"); | ||
| if msgtype_opt.is_none() { | ||
| eprintln!("msgtype attribute not found in message : {:?}", attributes); | ||
| return; | ||
| } | ||
| let msgtype = msgtype_opt.expect("Empty msgtype"); | ||
| match msgtype.as_str() { | ||
| "install_callback" => { | ||
| println!("Processing install callback message"); | ||
| let msg_data_res = serde_json::from_slice::<InstallCallback>(data_bytes); | ||
| if msg_data_res.is_err() { | ||
| eprintln!("Error deserializing install callback: {:?}", msg_data_res); | ||
| return; | ||
| } | ||
| let data = msg_data_res.expect("msg_data not found"); | ||
| let code_async = data.installation_code.clone(); | ||
| task::spawn(async move { | ||
| handle_install_bitbucket(&code_async).await; | ||
| println!("Processed install callback message"); | ||
| }); | ||
| }, | ||
| "webhook_callback" => { | ||
| let data_bytes_async = data_bytes.to_owned(); | ||
| task::spawn(async move { | ||
| process_review(&data_bytes_async).await; | ||
| println!("Processed webhook callback message"); | ||
| }); | ||
| } | ||
| _ => { | ||
| eprintln!("Message type not found for message : {:?}", attributes); | ||
| } | ||
| }; | ||
| } |
There was a problem hiding this comment.
The process_message function is handling two types of messages: "install_callback" and "webhook_callback". The processing of these messages is done asynchronously using tokio::task::spawn. However, the error handling for these spawned tasks is missing. If an error occurs during the execution of these tasks, it will be silently ignored. Consider adding error handling to these tasks.
task::spawn(async move {
match handle_install_bitbucket(&code_async).await {
Ok(_) => println!("Processed install callback message"),
Err(e) => eprintln!("Error processing install callback message: {:?}", e),
}
});
task::spawn(async move {
match process_review(&data_bytes_async).await {
Ok(_) => println!("Processed webhook callback message"),
Err(e) => eprintln!("Error processing webhook callback message: {:?}", e),
}
});
vibi-dpu/src/pubsub/listener.rs
Outdated
| async fn setup_subscription(keypath: &str, topicname: &str) -> Subscription{ | ||
| let config = get_pubsub_client_config(keypath).await; | ||
| let client = Client::new(config).await | ||
| .expect("Unable to create pubsub client to listen to messages"); | ||
| let topic = client.topic(topicname); | ||
| let topic_res = topic.exists(None).await; | ||
| if topic_res.is_err() { | ||
| let e = topic_res.expect_err("No error found in topic_res"); | ||
| if e.code() == Code::NotFound { | ||
| client.create_topic(topicname, None, None).await | ||
| .expect("Unable to create topic"); | ||
| } | ||
| else { | ||
| eprintln!("Error getting topic: {:?}", e); | ||
| } | ||
| } | ||
| let sub_config = SubscriptionConfig { | ||
| enable_message_ordering: true, | ||
| ..Default::default() | ||
| }; | ||
| let subscription_name = format!("{topicname}-sub"); | ||
| let subscription = client.subscription(&subscription_name); | ||
| let subconfig = SubscriptionConfig { | ||
| enable_message_ordering: true, | ||
| ..Default::default() | ||
| }; | ||
| if !subscription.exists(None).await.expect("Unable to get subscription information") { | ||
| subscription.create( | ||
| topic.fully_qualified_name(), subconfig, None) | ||
| .await.expect("Unable to create subscription for listening to messages"); | ||
| } | ||
| println!("sub = {:?}", &subscription); | ||
| subscription | ||
| } |
There was a problem hiding this comment.
The setup_subscription function creates a new topic if it does not exist (lines 77-78). However, there is no error handling for the case when the topic creation fails. This could lead to a situation where the function continues to execute even though the topic was not created successfully, which would likely cause errors later on. Consider adding error handling for this scenario.
if e.code() == Code::NotFound {
match client.create_topic(topicname, None, None).await {
Ok(_) => (),
Err(e) => {
eprintln!("Unable to create topic: {:?}", e);
return;
},
}
}
vibi-dpu/src/pubsub/listener.rs
Outdated
| pub async fn listen_messages(keypath: &str, topicname: &str) { | ||
| let queue_cap = 100; | ||
| let mut message_hashes = VecDeque::with_capacity(queue_cap); | ||
| let subscription = setup_subscription(keypath, topicname).await; | ||
| let mut stream = subscription.subscribe(None).await | ||
| .expect("Unable to subscribe to messages"); | ||
| while let Some(message) = stream.next().await { | ||
| println!("Listening for messages..."); | ||
| let attrmap: HashMap<String, String> = message.message.attributes.clone().into_iter().collect(); | ||
| let message_hash = digest(&*message.message.data); | ||
| if !message_hashes.contains(&message_hash) { | ||
| message_hashes.push_back(message_hash); | ||
| if message_hashes.len() > queue_cap { | ||
| while message_hashes.len() > queue_cap { | ||
| message_hashes.pop_front(); | ||
| } | ||
| } | ||
| let msg_bytes = message.message.data.clone(); | ||
| process_message(&attrmap, &msg_bytes).await; | ||
| } | ||
| // Ack or Nack message. | ||
| let _ = message.ack().await; | ||
| } | ||
| } |
There was a problem hiding this comment.
In the listen_messages function, you are maintaining a queue of message hashes to avoid processing duplicate messages. However, this approach has a limitation in that it only prevents duplicates within the most recent queue_cap messages. If a duplicate message arrives after more than queue_cap messages have been processed, it will be processed again. If this is a concern, you might want to consider a different approach for preventing duplicate message processing.
vibi-dpu/src/bitbucket/auth.rs
Outdated
| let bitbucket_client_id = env::var("BITBUCKET_CLIENT_ID").unwrap(); | ||
| let bitbucket_client_secret = env::var("BITBUCKET_CLIENT_SECRET").unwrap(); |
There was a problem hiding this comment.
Environment variables are directly unwrapped without checking if they exist or not. This could lead to a panic at runtime if the environment variables are not set. It would be better to handle this gracefully.
- let bitbucket_client_id = env::var("BITBUCKET_CLIENT_ID").unwrap();
- let bitbucket_client_secret = env::var("BITBUCKET_CLIENT_SECRET").unwrap();
+ let bitbucket_client_id = match env::var("BITBUCKET_CLIENT_ID") {
+ Ok(val) => val,
+ Err(_) => {
+ eprintln!("BITBUCKET_CLIENT_ID is not set");
+ return None;
+ }
+ };
+ let bitbucket_client_secret = match env::var("BITBUCKET_CLIENT_SECRET") {
+ Ok(val) => val,
+ Err(_) => {
+ eprintln!("BITBUCKET_CLIENT_SECRET is not set");
+ return None;
+ }
+ };| fn process_statoutput(statstr: &str) -> Option<(Vec<StatItem>, Vec<StatItem>)>{ | ||
| let statvec = process_statitems(statstr); | ||
| let mut bigfiles = Vec::<StatItem>::new(); | ||
| let mut smallfiles = Vec::<StatItem>::new(); | ||
| let line_threshold = 500; | ||
| for item in statvec { | ||
| // logic for exclusion | ||
| if (item.additions > line_threshold) || | ||
| (item.deletions > line_threshold) || | ||
| (item.additions + item.deletions > line_threshold) { | ||
| bigfiles.push(item); | ||
| } | ||
| else { | ||
| smallfiles.push(item); | ||
| } | ||
| } | ||
| return Some((bigfiles, smallfiles)); | ||
| } |
There was a problem hiding this comment.
In the process_statoutput function, you are using a hardcoded threshold value of 500 to separate big files from small files based on the number of additions and deletions. It would be better to make this threshold configurable so that it can be easily adjusted based on the needs of your application.
- let line_threshold = 500;
+ let line_threshold = env::var("LINE_THRESHOLD").unwrap_or(500);| fn generate_statitem(statitems: &Vec<&str>) -> StatItem { | ||
| let mut additions = 0; | ||
| let statitem = StatItem { | ||
| filepath: statitems[2].to_string(), | ||
| additions: match statitems[0].to_string().parse() { | ||
| Ok(adds) => {adds} | ||
| Err(e) => { | ||
| eprintln!("Unable to parse additions: {:?}", e); | ||
| 0 // default value | ||
| } | ||
| }, | ||
| deletions: match statitems[1].to_string().parse() { | ||
| Ok(dels) => {dels} | ||
| Err(e) => { | ||
| eprintln!("Unable to parse deletions: {:?}", e); | ||
| 0 // default value | ||
| } | ||
| }, | ||
| }; | ||
| return statitem; | ||
| } |
There was a problem hiding this comment.
The generate_statitem function attempts to parse the number of additions and deletions as integers. If parsing fails, it defaults to 0 and logs an error message. However, it might be more appropriate to return an error in this case, as a failure to parse these values likely indicates a problem with the input data.
| pub fn generate_diff(review: &Review, smallfiles: &Vec<StatItem>) -> HashMap<String, String> { | ||
| let mut diffmap = HashMap::<String, String>::new(); | ||
| let prev_commit = review.base_head_commit(); | ||
| let curr_commit = review.pr_head_commit(); | ||
| let clone_dir = review.clone_dir(); | ||
| for item in smallfiles { | ||
| let filepath = item.filepath.as_str(); | ||
| let params = vec![ | ||
| "diff".to_string(), | ||
| format!("{prev_commit}:{filepath}"), | ||
| format!("{curr_commit}:{filepath}"), | ||
| "-U0".to_string(), | ||
| ]; | ||
| let output_res = Command::new("git").args(¶ms) | ||
| .current_dir(&clone_dir) | ||
| .output(); | ||
| if output_res.is_err() { | ||
| let commanderr = output_res.expect_err("No error in output_res"); | ||
| eprintln!("git diff command failed to start : {:?}", commanderr); | ||
| continue; | ||
| } | ||
| let result = output_res.expect("Uncaught error in output_res"); | ||
| let diff = result.stdout; | ||
| let diffstr_res = str::from_utf8(&diff); | ||
| if diffstr_res.is_err() { | ||
| let e = diffstr_res.expect_err("No error in diffstr_res"); | ||
| eprintln!("Unable to deserialize diff: {:?}", e); | ||
| continue; | ||
| } | ||
| let diffstr = diffstr_res.expect("Uncaught error in diffstr_res"); | ||
| println!("diffstr = {}", &diffstr); | ||
| diffmap.insert(filepath.to_string(), diffstr.to_string()); | ||
| } | ||
| return diffmap; | ||
| } |
There was a problem hiding this comment.
The generate_diff function generates diffs for each file in the smallfiles vector. However, if the git diff command fails for any reason (e.g., the git command is not found), the function simply logs an error message and continues processing the remaining files. This could result in incomplete or incorrect results. Consider handling this error more robustly, for example by returning an error or stopping execution.
| pub fn process_diffmap(diffmap: &HashMap<String, String>) -> HashMap<String, Vec<String>> { | ||
| let mut linemap: HashMap<String, Vec<String>> = HashMap::new(); | ||
| for (filepath, diff) in diffmap { | ||
| linemap = process_diff(filepath, diff, &mut linemap); | ||
| } | ||
| return linemap; | ||
| } |
There was a problem hiding this comment.
The process_diffmap function processes a map of diffs. However, if processing a particular diff fails, the function simply logs an error message and continues processing the remaining diffs. This could result in incomplete or incorrect results. Consider handling this error more robustly, for example by returning an error or stopping execution.
vibi-dpu/src/utils/gitops.rs
Outdated
| pub async fn generate_blame(review: &Review, linemap: &HashMap<String, Vec<String>>) -> Vec<BlameItem>{ | ||
| let mut blamevec = Vec::<BlameItem>::new(); | ||
| let commit = review.pr_head_commit(); | ||
| let clone_dir = review.clone_dir(); | ||
| for (path, linevec) in linemap { | ||
| for line in linevec { | ||
| let linenumvec: Vec<&str> = line.split(",").collect(); | ||
| let linenum = linenumvec[0]; | ||
| let paramvec: Vec<&str> = vec!( | ||
| "blame", | ||
| commit, | ||
| "-L", | ||
| line.as_str(), | ||
| "-e", | ||
| "--date=unix", | ||
| path.as_str(), | ||
| ); | ||
| let blame_res = Command::new("git").args(paramvec) | ||
| .current_dir(clone_dir) | ||
| .output(); | ||
| if blame_res.is_err() { | ||
| let e = blame_res.expect_err("No error in blame_res"); | ||
| eprintln!("git blame command failed to start : {e}"); | ||
| continue; | ||
| } | ||
| let blame_output = blame_res.expect("Uncaught error in blame_res"); | ||
| if !blame_output.status.success() { | ||
| eprintln!("git blame command failed with exit code {:?} and error: {:?}", | ||
| blame_output.status.code(), String::from_utf8_lossy(&blame_output.stderr)); | ||
| continue; | ||
| } | ||
| let blame = blame_output.stdout; | ||
| let parse_res = str::from_utf8(&blame); | ||
| if parse_res.is_err() { | ||
| let e = parse_res.expect_err("No error in parse_res"); | ||
| eprintln!("Unable to deserialize blame: {e}"); | ||
| } | ||
| let blamestr = parse_res.expect("Uncaught error in parse_res"); | ||
| println!("blamestr = {}", blamestr); | ||
| let blamelines: Vec<&str> = blamestr.lines().collect(); | ||
| if blamelines.len() == 0 { | ||
| continue; | ||
| } | ||
| let blamitems_opt = process_blameitem(path, commit, linenum, blamelines, review).await; | ||
| if blamitems_opt.is_some() { | ||
| let blameitems = blamitems_opt.expect("blameitem not found in blameitem_opt"); | ||
| blamevec.extend(blameitems); | ||
| } | ||
| } | ||
| } | ||
| return blamevec; | ||
| } |
There was a problem hiding this comment.
The generate_blame function generates blame information for each line in each file in the linemap. However, if the git blame command fails for any reason (e.g., the git command is not found), the function simply logs an error message and continues processing the remaining lines/files. This could result in incomplete or incorrect results. Consider handling this error more robustly, for example by returning an error or stopping execution.
vibi-dpu/src/pubsub/listener.rs
Outdated
| @@ -12,8 +12,7 @@ use tokio::task; | |||
| use std::collections::VecDeque; | |||
| use sha256::digest; | |||
| use tonic::Code; | |||
| use crate::core::setup::handle_install_bitbucket; | |||
| // use crate::core::{setup::handle_install_bitbucket, review::process_review}; // To be added in future PR | |||
| use crate::core::{setup::handle_install_bitbucket, review::process_review}; // To be added in future PR | |||
There was a problem hiding this comment.
| use crate::core::{setup::handle_install_bitbucket, review::process_review}; // To be added in future PR | |
| use crate::core::{setup::handle_install_bitbucket, review::process_review}; |
vibi-dpu/src/core/review.rs
Outdated
| println!("Hunkmap published successfully!"); | ||
| }, | ||
| Err(e) => { | ||
| eprintln!("Failed to publish hunkmap: {}", e); | ||
| } |
There was a problem hiding this comment.
These are running asynchronously. We will not be able to associate which log message is related to which URL. Please add the URL to t he log messages.
| if line.contains("\n") || sublines.len() != 4 { | ||
| idx += 1; | ||
| continue; | ||
| } |
There was a problem hiding this comment.
Please add a comment mentioning which edge case are you handling by this.
| let ln = blamelines[lnum]; | ||
| let wordvec: Vec<&str> = ln.split(" ").collect(); | ||
| let commit = wordvec[0]; | ||
| let lineitem_opt = get_commit_bb(commit, repo_name, repo_owner).await; |
There was a problem hiding this comment.
This is a repo-provider-specific function call, so there should have been a conditional based on the repo provider before calling this function.
This will create confusion going forward when we implement this for other providers and treat the functions in gitops as black boxes (as we should).
| review.author().to_string(), | ||
| blamevec, | ||
| ); | ||
| prvec.push(hmapitem); |
There was a problem hiding this comment.
You only pushing once in this vector. You could have directly defined the variable here instead of creating a mutable.
There was a problem hiding this comment.
This change has too many cascading effects, will need to change how tmw parses hunks as well
There was a problem hiding this comment.
Review Status
Actionable comments generated: 7
Files selected for processing (2)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/pubsub/listener.rs (2 hunks)
Additional comments (Suppressed): 2
vibi-dpu/src/pubsub/listener.rs (2)
12-15: The import of
process_reviewfunction fromreviewmodule incoreis added. This function is used to process the review tasks. Ensure that the function is implemented correctly and handles all the necessary tasks as expected.44-52: The commented out code for processing the "webhook_callback" message type is now uncommented and updated. The
process_reviewfunction is now called withdata_bytes_asyncas an argument. Ensure that theprocess_reviewfunction handles the data correctly and that the data is in the expected format.
vibi-dpu/src/core/review.rs
Outdated
| fn publish_hunkmap(hunkmap: &HunkMap) { | ||
| let client = get_client(); | ||
| let hunkmap_json = serde_json::to_string(&hunkmap).expect("Unable to serialize hunkmap"); | ||
| tokio::spawn(async move { | ||
| let url = format!("{}/api/hunks", | ||
| env::var("SERVER_URL").expect("SERVER_URL must be set")); | ||
| let url_clone = url.clone(); | ||
| println!("url for hunkmap publishing {}", &url); | ||
| match client | ||
| .post(url) | ||
| .json(&hunkmap_json) | ||
| .send() | ||
| .await { | ||
| Ok(_) => { | ||
| println!("[publish_hunkmap] Hunkmap published successfully for: {} !", &url_clone); | ||
| }, | ||
| Err(e) => { | ||
| eprintln!("[publish_hunkmap] Failed to publish hunkmap: {} for: {}", e, &url_clone); | ||
| } | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The function publish_hunkmap sends a POST request to publish the hunkmap. However, it does not handle any errors that might occur during the request. Consider adding error handling to this function to improve the robustness of the code.
|
Kudos, SonarCloud Quality Gate passed!
|
There was a problem hiding this comment.
Review Status
Actionable comments generated: 9
Files selected for processing (2)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/utils/gitops.rs (1 hunks)
Additional comments (Suppressed): 1
vibi-dpu/src/utils/gitops.rs (1)
- 94-122: The function
set_git_urlsets the URL of the remote repository. However, it doesn't handle the case where the git command fails due to reasons other
| pub async fn process_review(message_data: &Vec<u8>) { | ||
| let review_opt = parse_review(message_data); | ||
| if review_opt.is_none() { | ||
| eprintln!("Unable to deserialize review message"); | ||
| return; | ||
| } | ||
| let review = review_opt.expect("review_opt is empty"); | ||
| if hunk_already_exists(&review) { | ||
| return; | ||
| } | ||
| println!("Processing PR : {}", &review.id()); | ||
| commit_check(&review).await; | ||
| let hunkmap_opt = process_review_changes(&review).await; | ||
| send_hunkmap(&hunkmap_opt, &review); | ||
| } |
There was a problem hiding this comment.
The function process_review does not handle errors effectively. If parse_review returns None, it simply prints an error message and continues execution. This could lead to unexpected behavior if review_opt is None. Consider returning an error or exiting the function early in this case. Also, the function commit_check does not return any value or error, which makes it difficult to handle any potential errors that might occur during its execution. Consider modifying this function to return a Result type.
pub async fn process_review(message_data: &Vec<u8>) {
let review_opt = parse_review(message_data);
if review_opt.is_none() {
eprintln!("Unable to deserialize review message");
return;
}
let review = review_opt.expect("review_opt is empty");
if hunk_already_exists(&review) {
return;
}
println!("Processing PR : {}", &review.id());
- 30: commit_check(&review).await;
+ 30: if let Err(e) = commit_check(&review).await {
+ 31: eprintln!("Failed to check commit: {}", e);
+ 32: return;
+ 33: }
let hunkmap_opt = process_review_changes(&review).await;
send_hunkmap(&hunkmap_opt, &review);
}| fn send_hunkmap(hunkmap_opt: &Option<HunkMap>, review: &Review) { | ||
| if hunkmap_opt.is_none() { | ||
| eprintln!("Empty hunkmap in send_hunkmap"); | ||
| return; | ||
| } | ||
| let hunkmap = hunkmap_opt.to_owned().expect("empty hunkmap_opt"); | ||
| println!("HunkMap = {:?}", &hunkmap); | ||
| store_hunkmap_to_db(&hunkmap, review); | ||
| publish_hunkmap(&hunkmap); | ||
| // let hunkmap_async = hunkmap.clone(); | ||
| // process_coverage(&hunkmap_async).await; TODO - include in future PR | ||
| } |
There was a problem hiding this comment.
The function send_hunkmap does not handle the case where hunkmap_opt is None effectively. It simply prints an error message and continues execution. This could lead to unexpected behavior if hunkmap_opt is None. Consider returning an error or exiting the function early in this case.
fn send_hunkmap(hunkmap_opt: &Option<HunkMap>, review: &Review) {
if hunkmap_opt.is_none() {
eprintln!("Empty hunkmap in send_hunkmap");
return;
}
let hunkmap = hunkmap_opt.to_owned().expect("empty hunkmap_opt");
println!("HunkMap = {:?}", &hunkmap);
store_hunkmap_to_db(&hunkmap, review);
publish_hunkmap(&hunkmap);
// let hunkmap_async = hunkmap.clone();
// process_coverage(&hunkmap_async).await; TODO - include in future PR
}| fn hunk_already_exists(review: &Review) -> bool { | ||
| let hunk_opt = get_hunk_from_db(&review); | ||
| if hunk_opt.is_none() { | ||
| eprintln!("No hunk from get_hunk_from_db"); | ||
| return false; | ||
| } | ||
| let hunkmap = hunk_opt.expect("empty hunk from get_hunk_from_db"); | ||
| publish_hunkmap(&hunkmap); | ||
| println!("Hunk already in db!"); | ||
| return true; | ||
| } |
There was a problem hiding this comment.
The function hunk_already_exists returns true if a hunk already exists in the database, but it also publishes the hunkmap. This could lead to unnecessary network traffic if the hunkmap is published multiple times. Consider separating the concerns of checking if a hunk exists and publishing a hunkmap into separate functions.
| async fn process_review_changes(review: &Review) -> Option<HunkMap>{ | ||
| let mut prvec = Vec::<PrHunkItem>::new(); | ||
| let fileopt = get_excluded_files(&review); | ||
| println!("fileopt = {:?}", &fileopt); | ||
| if fileopt.is_none() { | ||
| eprintln!("No files to review for PR {}", review.id()); | ||
| return None; | ||
| } | ||
| let (_, smallfiles) = fileopt.expect("fileopt is empty"); | ||
| let diffmap = generate_diff(&review, &smallfiles); | ||
| println!("diffmap = {:?}", &diffmap); | ||
| let linemap = process_diffmap(&diffmap); | ||
| let blamevec = generate_blame(&review, &linemap).await; | ||
| let hmapitem = PrHunkItem::new( | ||
| review.id().to_string(), | ||
| review.author().to_string(), | ||
| blamevec, | ||
| ); | ||
| prvec.push(hmapitem); | ||
| let hunkmap = HunkMap::new(review.provider().to_string(), | ||
| review.repo_owner().to_string(), | ||
| review.repo_name().to_string(), | ||
| prvec, | ||
| format!("{}/hunkmap", review.db_key()), | ||
| ); | ||
| return Some(hunkmap); | ||
| } |
There was a problem hiding this comment.
The function process_review_changes is doing too many things: it's generating a diff, processing the diffmap, generating blame, and creating a hunkmap. Consider breaking this function into smaller, more manageable functions to improve readability and maintainability.
| async fn commit_check(review: &Review) { | ||
| if !commit_exists(&review.base_head_commit(), &review.clone_dir()) | ||
| || !commit_exists(&review.pr_head_commit(), &review.clone_dir()) { | ||
| println!("Pulling repository {} for commit history", &review.repo_name()); | ||
| git_pull(review).await; | ||
| } | ||
| } |
There was a problem hiding this comment.
The function commit_check does not return any value or error, which makes it difficult to handle any potential errors that might occur during its execution. Consider modifying this function to return a Result type.
| fn parse_review(message_data: &Vec<u8>) -> Option<Review>{ | ||
| let data_res = serde_json::from_slice::<Value>(&message_data); | ||
| if data_res.is_err() { | ||
| let e = data_res.expect_err("No error in data_res"); | ||
| eprintln!("Incoming message does not contain valid reviews: {:?}", e); | ||
| return None; | ||
| } | ||
| let data = data_res.expect("Uncaught error in deserializing message_data"); | ||
| println!("data == {:?}", &data["eventPayload"]["repository"]); | ||
| let repo_provider = data["repositoryProvider"].to_string().trim_matches('"').to_string(); | ||
| let repo_name = data["eventPayload"]["repository"]["name"].to_string().trim_matches('"').to_string(); | ||
| println!("repo NAME == {}", &repo_name); | ||
| let workspace_name = data["eventPayload"]["repository"]["workspace"]["slug"].to_string().trim_matches('"').to_string(); | ||
| let clone_opt = get_clone_url_clone_dir(&repo_provider, &workspace_name, &repo_name); | ||
| if clone_opt.is_none() { | ||
| eprintln!("Unable to get clone url and directory"); | ||
| return None; | ||
| } | ||
| let (clone_url, clone_dir) = clone_opt.expect("Empty clone_opt"); | ||
| let pr_id = data["eventPayload"]["pullrequest"]["id"].to_string().trim_matches('"').to_string(); | ||
| let review = Review::new( | ||
| data["eventPayload"]["pullrequest"]["destination"]["commit"]["hash"].to_string().replace("\"", ""), | ||
| data["eventPayload"]["pullrequest"]["source"]["commit"]["hash"].to_string().replace("\"", ""), | ||
| pr_id.clone(), | ||
| repo_name.clone(), | ||
| workspace_name.clone(), | ||
| repo_provider.clone(), | ||
| format!("bitbucket/{}/{}/{}", &workspace_name, &repo_name, &pr_id), | ||
| clone_dir, | ||
| clone_url, | ||
| data["eventPayload"]["pullrequest"]["author"]["account_id"].to_string().replace("\"", ""), | ||
| ); | ||
| println!("review = {:?}", &review); | ||
| save_review_to_db(&review); | ||
| return Some(review); | ||
| } |
There was a problem hiding this comment.
The function parse_review is doing too many things: it's deserializing the message data, extracting various pieces of information, getting the clone URL and directory, and creating a review. Consider breaking this function into smaller, more manageable functions to improve readability and maintainability.
| fn publish_hunkmap(hunkmap: &HunkMap) { | ||
| let client = get_client(); | ||
| let hunkmap_json = serde_json::to_string(&hunkmap).expect("Unable to serialize hunkmap"); | ||
| let key_clone = hunkmap.db_key().to_string(); | ||
| tokio::spawn(async move { | ||
| let url = format!("{}/api/hunks", | ||
| env::var("SERVER_URL").expect("SERVER_URL must be set")); | ||
| println!("url for hunkmap publishing {}", &url); | ||
| match client | ||
| .post(url) | ||
| .json(&hunkmap_json) | ||
| .send() | ||
| .await { | ||
| Ok(_) => { | ||
| println!("[publish_hunkmap] Hunkmap published successfully for: {} !", &key_clone); | ||
| }, | ||
| Err(e) => { | ||
| eprintln!("[publish_hunkmap] Failed to publish hunkmap: {} for: {}", e, &key_clone); | ||
| } | ||
| }; | ||
| }); | ||
| } |
There was a problem hiding this comment.
The function publish_hunkmap is doing too many things: it's serializing the hunkmap, creating a URL, and sending a POST request. Consider breaking this function into smaller, more manageable functions to improve readability and maintainability.
| pub fn commit_exists(commit: &str, directory: &str) -> bool { | ||
| let output_res = Command::new("git") | ||
| .arg("rev-list") | ||
| .arg(commit) | ||
| .current_dir(directory) | ||
| .output(); | ||
| if output_res.is_err() { | ||
| let e = output_res.expect_err("No error in output_res"); | ||
| eprintln!("Failed to start git rev-list: {:?}", e); | ||
| return false; | ||
| } | ||
| let output = output_res.expect("Uncaught error in output_res"); | ||
| if !output.status.success() { | ||
| eprintln!("git rev-list, exit code: {:?}", | ||
| output.status.code()); | ||
| // for debugging | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => println!("git rev-list stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git rev-list stderr error {}", e)}, | ||
| }; | ||
| return false; | ||
| } | ||
| println!("Execute git rev-list, exit code: {:?}", output.status.code()); | ||
| if output.status.code() == Some(128) { | ||
| // for debugging | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => eprintln!("git rev-list stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ eprintln!("git rev-list stderr error {}", e)}, | ||
| }; | ||
| return false; | ||
| } | ||
| // for debugging | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => eprintln!("git rev-list stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ eprintln!("git rev-list stderr error {}", e)}, | ||
| }; | ||
| match str::from_utf8(&output.stdout) { | ||
| Ok(v) => println!("git rev-list stdout = {:?}", v), | ||
| Err(e) => {/* error handling */ eprintln!("git rev-list stdout error {}", e)}, | ||
| }; | ||
| return true; |
There was a problem hiding this comment.
The function commit_exists checks if a commit exists in a given directory. However, it doesn't handle the case where the git command fails due to reasons other than the commit not existing (e.g., the git command is not found). This could lead to unexpected behavior. Consider checking the error message or exit code to distinguish between different failure cases. Also, the function prints a lot of debug information to the console. Consider using a logging library to control the verbosity of the output.
pub fn commit_exists(commit: &str, directory: &str) -> bool {
let output_res = Command::new("git")
.arg("rev-list")
.arg(commit)
.current_dir(directory)
.output();
if output_res.is_err() {
let e = output_res.expect_err("No error in output_res");
eprintln!("Failed to start git rev-list: {:?}", e);
return false;
}
let output = output_res.expect("Uncaught error in output_res");
if !output.status.success() {
eprintln!("git rev-list, exit code: {:?}",
output.status.code());
// for debugging
match str::from_utf8(&output.stderr) {
Ok(v) => println!("git rev-list stderr = {:?}", v),
Err(e) => {/* error handling */ println!("git rev-list stderr error {}", e)},
};
return false;
}
println!("Execute git rev-list, exit code: {:?}", output.status.code());
if output.status.code() == Some(128) {
// for debugging
match str::from_utf8(&output.stderr) {
Ok(v) => eprintln!("git rev-list stderr = {:?}", v),
Err(e) => {/* error handling */ eprintln!("git rev-list stderr error {}", e)},
};
return false;
}
// for debugging
match str::from_utf8(&output.stderr) {
Ok(v) => eprintln!("git rev-list stderr = {:?}", v),
Err(e) => {/* error handling */ eprintln!("git rev-list stderr error {}", e)},
};
match str::from_utf8(&output.stdout) {
Ok(v) => println!("git rev-list stdout = {:?}", v),
Err(e) => {/* error handling */ eprintln!("git rev-list stdout error {}", e)},
};
return true;
}| pub async fn git_pull(review: &Review) { | ||
| let directory = review.clone_dir(); | ||
| println!("directory = {}", &directory); | ||
| let access_token_opt = refresh_git_auth(review.clone_url(), review.clone_dir()).await; | ||
| if access_token_opt.is_none() { | ||
| eprintln!("no refresh token acquired"); | ||
| } | ||
| let access_token = access_token_opt.expect("Empty access_token"); | ||
| set_git_url(review.clone_url(), directory, &access_token); | ||
| let output_res = Command::new("git") | ||
| .arg("pull") | ||
| .current_dir(directory) | ||
| .output(); | ||
| if output_res.is_err() { | ||
| let e = output_res.expect_err("No error in output_res"); | ||
| eprintln!("failed to execute git pull: {:?}", e); | ||
| return; | ||
| } | ||
| let output = output_res.expect("Uncaught error in output_res"); | ||
| match str::from_utf8(&output.stderr) { | ||
| Ok(v) => println!("git pull stderr = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git pull stderr error {}", e)}, | ||
| }; | ||
| match str::from_utf8(&output.stdout) { | ||
| Ok(v) => println!("git pull stdout = {:?}", v), | ||
| Err(e) => {/* error handling */ println!("git pull stdout error {}", e)}, | ||
| }; | ||
| } |
There was a problem hiding this comment.
The function git_pull pulls the latest changes from a git repository. However, it doesn't handle the case where the git command fails due to reasons other than the commit not existing (e.g., the git command is not found). This could lead to unexpected behavior. Consider checking the error message or exit code to distinguish between different failure cases. Also, the function prints a lot of debug information to the console. Consider using a logging library to control the verbosity of the output.
pub async fn git_pull(review: &Review) {
let directory = review.clone_dir();
println!("directory = {}", &directory);
let access_token_opt = refresh_git_auth(review.clone_url(), review.clone_dir()).await;
if access_token_opt.is_none() {
eprintln!("no refresh token acquired");
}
let access_token = access_token_opt.expect("Empty access_token");
set_git_url(review.clone_url(), directory, &access_token);
let output_res = Command::new("git")
.arg("pull")
.current_dir(directory)
.output();
if output_res.is_err() {
let e = output_res.expect_err("No error in output_res");
eprintln!("failed to execute git pull: {:?}", e);
return;
}
let output = output_res.expect("Uncaught error in output_res");
match str::from_utf8(&output.stderr) {
Ok(v) => println!("git pull stderr = {:?}", v),
Err(e) => {/* error handling */ println!("git pull stderr error {}", e)},
};
match str::from_utf8(&output.stdout) {
Ok(v) => println!("git pull stdout = {:?}", v),
Err(e) => {/* error handling */ println!("git pull stdout error {}", e)},
};
}
This code implements the following scenario -
User pushes to pull request
-> next server webhook callback receives event
-> dpu receives pubsub message
-> dpu does a git pull to get latest code
->git stat command is run and big files are eliminated
-> git diff command is run and output parsed
-> git blame command is run and parsed
-> hunk authors are obtained from bb api
-> Packaged and sent to next server by calling api
I would recommend starting in pubsub/listener.rs and then following the code
Summary by CodeRabbit