Initial setup for adding GitHub on-prem support. #65
Conversation
… all imports accordingly.
Initialization for github onprem
Added missing github specific args in dockerfile
WalkthroughThe changes primarily revolve around the integration of GitHub and Bitbucket into the codebase. This includes the addition of new modules, modification of existing ones, and the introduction of new environment variables in the Dockerfile. The Changes
TipsChat with CodeRabbit Bot (
|
|
SonarCloud Quality Gate failed.
|
![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: 3
Configuration used: CodeRabbit UI
Files ignored due to filter (1)
- vibi-dpu/Cargo.toml
Files selected for processing (19)
- Dockerfile (2 hunks)
- vibi-dpu/src/bitbucket/auth.rs (1 hunks)
- vibi-dpu/src/bitbucket/comment.rs (1 hunks)
- vibi-dpu/src/bitbucket/config.rs (1 hunks)
- vibi-dpu/src/bitbucket/prs.rs (1 hunks)
- vibi-dpu/src/bitbucket/reviewer.rs (1 hunks)
- vibi-dpu/src/bitbucket/webhook.rs (1 hunks)
- vibi-dpu/src/core/bitbucket/mod.rs (1 hunks)
- vibi-dpu/src/core/bitbucket/setup.rs (2 hunks)
- vibi-dpu/src/core/github/mod.rs (1 hunks)
- vibi-dpu/src/core/github/setup.rs (1 hunks)
- vibi-dpu/src/core/mod.rs (1 hunks)
- vibi-dpu/src/core/review.rs (1 hunks)
- vibi-dpu/src/github/auth.rs (1 hunks)
- vibi-dpu/src/github/mod.rs (1 hunks)
- vibi-dpu/src/main.rs (1 hunks)
- vibi-dpu/src/pubsub/listener.rs (1 hunks)
- vibi-dpu/src/utils/mod.rs (1 hunks)
- vibi-dpu/src/utils/reqwest_client.rs (1 hunks)
Files skipped from review due to trivial changes (10)
- vibi-dpu/src/bitbucket/comment.rs
- vibi-dpu/src/bitbucket/config.rs
- vibi-dpu/src/core/bitbucket/mod.rs
- vibi-dpu/src/core/github/mod.rs
- vibi-dpu/src/core/mod.rs
- vibi-dpu/src/github/mod.rs
- vibi-dpu/src/main.rs
- vibi-dpu/src/pubsub/listener.rs
- vibi-dpu/src/utils/mod.rs
- vibi-dpu/src/utils/reqwest_client.rs
Additional comments: 13
vibi-dpu/src/bitbucket/reviewer.rs (1)
- 3-11: The changes in the import statements and the function signature of
add_reviewersare consistent with the PR summary. Ensure that all calls toadd_reviewersthroughout the codebase have been updated to match the new signature.The rest of the code seems to be logically correct and does not have any apparent issues. However, it would be beneficial to add more comments to the code to improve readability and maintainability.
Dockerfile (3)
19-25: New build arguments have been added for GitHub integration. Ensure that these arguments are being passed correctly during the Docker build process.
36-39: New environment variables have been added for GitHub integration. Ensure that these variables are being used correctly in the application.
43-43: A new file
repo-profiler.pemis being copied to the/appdirectory. Ensure that this file is being used correctly and securely in the application.vibi-dpu/src/core/review.rs (1)
- 11-20: The import statements have been updated to use the
get_clientfunction from thereqwest_clientmodule instead of thebitbucket::config::get_client. Ensure that theget_clientfunction in thereqwest_clientmodule has the same functionality as the one in thebitbucket::configmodule.---end hunk 0---
The rest of the code seems to be logically correct and doesn't have any apparent issues. However, it's important to ensure that all the functions used in this file are updated and working as expected, especially after the changes in the import statements.
vibi-dpu/src/bitbucket/auth.rs (1)
- 2-9: The changes in the import statements are consistent with the PR summary. The
get_clientfunction is now being imported from thecrate::utils::reqwest_clientmodule, and thesave_auth_info_to_dbfunction is imported from thecrate::db::authmodule. Ensure that these changes are reflected across all files that use these functions.The rest of the code seems to be logically correct and doesn't have any apparent issues. However, it's important to ensure that the environment variables like "BITBUCKET_CLIENT_ID", "BITBUCKET_CLIENT_SECRET", and "SERVER_URL" are properly set in the environment where this code is running. If these variables are not set, the code will panic at runtime.
vibi-dpu/src/bitbucket/webhook.rs (2)
4-8: The import statements have been updated correctly. Ensure that the new modules and functions exist and are accessible from this file.
11-11: The function signature looks good. Ensure that all calls to this function throughout the codebase have been updated to match the new signature.
vibi-dpu/src/core/bitbucket/setup.rs (1)
- 7-13: The import statements are well organized and follow the Rust convention of separating standard library, third-party, and local crate imports.
vibi-dpu/src/core/github/setup.rs (1)
- 1-46: This is a good start for the GitHub setup module. However, there are several TODOs that need to be implemented. Also, consider error handling for the async functions. For instance, what happens if
fetch_access_tokenfails? You might want to return aResultfrom these functions to handle potential errors.pub async fn handle_install_github(installation_code: &str) -> Result<(), Box<dyn std::error::Error>> { // TODO: Implement the logic to handle GitHub installation // For example: // 1. Get access token from GitHub using the installation code let auth_info = fetch_access_token(installation_code).await?; println!("[handle_install_github] auth_info = {:?}", &auth_info); // 2. Fetch user repositories and other necessary data // 3. Process webhooks or other setup tasks // 4. Send setup info or any other post-processing Ok(()) }vibi-dpu/src/bitbucket/prs.rs (2)
2-11: The import statements and function signature look good. The function
list_prs_bitbucketnow uses theget_clientfunction from thereqwest_clientmodule, which is a good practice for code reusability.10-11: The function
list_prs_bitbucketreturns anOption<Vec<String>>. This is a good practice as it allows the function to returnNonein case of any errors, which can be handled by the caller.vibi-dpu/src/github/auth.rs (1)
- 1-7: The import statements look fine. Ensure that all the imported crates are included in the
Cargo.tomlfile.
| send_setup_info(&pubreqs).await; | ||
| } | ||
|
|
||
|
|
||
| async fn send_setup_info(setup_info: &Vec<SetupInfo>) { | ||
| let installation_id = env::var("INSTALL_ID") | ||
| .expect("INSTALL_ID must be set"); |
There was a problem hiding this comment.
The function send_setup_info is missing error handling for the case when the environment variables "INSTALL_ID" and "SERVER_URL" are not set. This could lead to a panic at runtime. Consider using env::var().ok() instead of env::var().expect(), and handle the None case appropriately.
let installation_id = env::var("INSTALL_ID").ok();
if installation_id.is_none() {
eprintln!("INSTALL_ID must be set");
return;
}
let base_url = env::var("SERVER_URL").ok();
if base_url.is_none() {
eprintln!("SERVER_URL must be set");
return;
}The rest of the code looks fine, but it's hard to review without the full context. It would be helpful to see how these functions are used in the larger application.
| fn generate_jwt(github_app_id: &str) -> Option<String> { | ||
| let pem_file_path = "/app/repoprofiler_private.pem"; | ||
| let pem_data_res = fs::read(pem_file_path); | ||
|
|
||
| if pem_data_res.is_err() { | ||
| let pem_data_err = pem_data_res.expect_err("No error in reading pem file"); | ||
| println!("Error reading pem file: {:?}", pem_data_err); | ||
| return None; | ||
| } | ||
| let pem_data = pem_data_res.expect("Error reading pem file"); | ||
|
|
||
| let my_claims = Claims { | ||
| iat: Utc::now().timestamp(), | ||
| exp: (Utc::now() + Duration::minutes(5)).timestamp(), | ||
| iss: github_app_id.to_string(), | ||
| }; | ||
|
|
||
| let encoding_key = EncodingKey::from_rsa_pem(&pem_data); | ||
| if encoding_key.is_err() { | ||
| println!("Error creating encoding key"); | ||
| return None; | ||
| } | ||
|
|
||
| let token_res = encode(&Header::new(Algorithm::RS256), &my_claims, &encoding_key.unwrap()); | ||
| if token_res.is_err() { | ||
| let token_err = token_res.expect_err("No error in fetching token"); | ||
| println!("Error encoding JWT: {:?}", token_err); | ||
| return None; | ||
| }; | ||
| let token = token_res.expect("Error encoding JWT"); | ||
| Some(token) | ||
| } |
There was a problem hiding this comment.
The generate_jwt function reads a private key from a file, creates a JWT token, and returns it. The function handles errors correctly and returns None if any step fails. However, it would be better to return a Result instead of an Option to propagate the error message to the caller. This would help in debugging if something goes wrong. Also, consider using the ? operator to simplify error handling.
fn generate_jwt(github_app_id: &str) -> Result<String, Box<dyn std::error::Error>> {
let pem_file_path = "/app/repoprofiler_private.pem";
let pem_data = fs::read(pem_file_path)?;
let my_claims = Claims {
iat: Utc::now().timestamp(),
exp: (Utc::now() + Duration::minutes(5)).timestamp(),
iss: github_app_id.to_string(),
};
let encoding_key = EncodingKey::from_rsa_pem(&pem_data)?;
let token = encode(&Header::new(Algorithm::RS256), &my_claims, &encoding_key)?;
Ok(token)
}| pub async fn fetch_access_token(installation_id: &str) -> Option<Value> { | ||
| let github_app_id = env::var("GITHUB_APP_ID"); | ||
| let github_app_id_str = github_app_id.expect("GITHUB_APP_ID must be set"); | ||
| let jwt_token = generate_jwt(&github_app_id_str).expect("Error generating JWT"); | ||
|
|
||
| let client = get_client(); | ||
| let response = client.post(&format!("https://api.github.com/app/installations/{}/access_tokens", installation_id)) | ||
| .header("Accept", "application/vnd.github+json") | ||
| .header("Authorization", format!("Bearer {}", jwt_token)) | ||
| .header("User-Agent", "Vibinex code review Test App") | ||
| .send() | ||
| .await; | ||
| if response.is_err() { | ||
| let e = response.expect_err("No error in response"); | ||
| eprintln!("error in calling github api : {:?}", e); | ||
| return None; | ||
| } | ||
| let response_access_token = response.expect("Uncaught error in reponse"); | ||
| if !response_access_token.status().is_success() { | ||
| println!( | ||
| "Failed to exchange code for access token. Status code: {}, Response content: {:?}", | ||
| response_access_token.status(), | ||
| response_access_token.text().await | ||
| ); | ||
| return None; | ||
| } | ||
| let parse_res = response_access_token.json().await ; | ||
| if parse_res.is_err() { | ||
| let e = parse_res.expect_err("No error in parse_res for AuthInfo"); | ||
| eprintln!("error deserializing AuthInfo: {:?}", e); | ||
| return None; | ||
| } | ||
| let response_json: Value = parse_res.expect("Uncaught error in parse_res for AuthInfo"); | ||
| return Some(response_json); | ||
| } |
There was a problem hiding this comment.
The fetch_access_token function fetches an access token from the GitHub API. It handles errors correctly and returns None if any step fails. However, similar to the generate_jwt function, it would be better to return a Result instead of an Option to propagate the error message to the caller. Also, consider using the ? operator to simplify error handling.
pub async fn fetch_access_token(installation_id: &str) -> Result<Value, Box<dyn std::error::Error>> {
let github_app_id = env::var("GITHUB_APP_ID")?;
let jwt_token = generate_jwt(&github_app_id)?;
let client = get_client();
let response = client.post(&format!("https://api.github.com/app/installations/{}/access_tokens", installation_id))
.header("Accept", "application/vnd.github+json")
.header("Authorization", format!("Bearer {}", jwt_token))
.header("User-Agent", "Vibinex code review Test App")
.send()
.await?;
if !response.status().is_success() {
return Err(format!(
"Failed to exchange code for access token. Status code: {}, Response content: {:?}",
response.status(),
response.text().await
).into());
}
let response_json: Value = response.json().await?;
return Ok(response_json);
}
|
All changes in branch |
Please check the action items covered in the PR -
Summary by CodeRabbit
New Features:
Refactor:
get_clientfunction in thereqwest_clientmodule, improving code maintainability.handle_install_bitbucketandprocess_reviewfunctions to more appropriate modules, enhancing code organization.Chores:
repo-profiler.pemfile to the Docker image.The changes aim to improve the application's versatility by supporting GitHub, enhance code maintainability, and update the Docker build process.