-
Notifications
You must be signed in to change notification settings - Fork 33
Add queue page #291
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add queue page #291
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
use anyhow::Error as AnyhowError; | ||
use axum::http::StatusCode; | ||
use axum::response::{IntoResponse, Response}; | ||
|
||
pub struct AppError(pub AnyhowError); | ||
|
||
impl IntoResponse for AppError { | ||
fn into_response(self) -> Response { | ||
let msg = format!("Something went wrong: {}", self.0); | ||
tracing::error!("{msg}"); | ||
(StatusCode::INTERNAL_SERVER_ERROR, msg).into_response() | ||
} | ||
} | ||
|
||
impl<E> From<E> for AppError | ||
where | ||
E: Into<AnyhowError>, | ||
{ | ||
fn from(err: E) -> Self { | ||
Self(err.into()) | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -4,18 +4,22 @@ use crate::bors::mergeable_queue::{ | |
handle_mergeable_queue_item, | ||
}; | ||
use crate::bors::{ | ||
BorsContext, RepositoryState, handle_bors_global_event, handle_bors_repository_event, | ||
BorsContext, RepositoryState, RollupMode, handle_bors_global_event, | ||
handle_bors_repository_event, | ||
}; | ||
use crate::github::webhook::GitHubWebhook; | ||
use crate::github::webhook::WebhookSecret; | ||
use crate::templates::{HtmlTemplate, IndexTemplate, NotFoundTemplate, RepositoryView}; | ||
use crate::templates::{ | ||
HelpTemplate, HtmlTemplate, NotFoundTemplate, PullRequestStats, QueueTemplate, RepositoryView, | ||
}; | ||
use crate::{BorsGlobalEvent, BorsRepositoryEvent, PgDbClient, TeamApiClient}; | ||
|
||
use super::AppError; | ||
use anyhow::Error; | ||
use axum::Router; | ||
use axum::extract::State; | ||
use axum::extract::{Path, State}; | ||
use axum::http::StatusCode; | ||
use axum::response::{IntoResponse, Response}; | ||
use axum::response::{IntoResponse, Redirect, Response}; | ||
use axum::routing::{get, post}; | ||
use octocrab::Octocrab; | ||
use std::any::Any; | ||
|
@@ -66,6 +70,8 @@ pub type ServerStateRef = Arc<ServerState>; | |
pub fn create_app(state: ServerState) -> Router { | ||
Router::new() | ||
.route("/", get(index_handler)) | ||
.route("/help", get(help_handler)) | ||
.route("/queue/{repo_name}", get(queue_handler)) | ||
.route("/github", post(github_webhook_handler)) | ||
.route("/health", get(health_handler)) | ||
.layer(ConcurrencyLimitLayer::new(100)) | ||
|
@@ -87,7 +93,11 @@ async fn health_handler() -> impl IntoResponse { | |
(StatusCode::OK, "") | ||
} | ||
|
||
async fn index_handler(State(state): State<ServerStateRef>) -> impl IntoResponse { | ||
async fn index_handler() -> impl IntoResponse { | ||
Redirect::permanent("/queue/rust") | ||
} | ||
|
||
async fn help_handler(State(state): State<ServerStateRef>) -> impl IntoResponse { | ||
let mut repos = Vec::with_capacity(state.repositories.len()); | ||
for repo in state.repositories.keys() { | ||
let treeclosed = state | ||
|
@@ -103,7 +113,49 @@ async fn index_handler(State(state): State<ServerStateRef>) -> impl IntoResponse | |
}); | ||
} | ||
|
||
HtmlTemplate(IndexTemplate { repos }) | ||
HtmlTemplate(HelpTemplate { repos }) | ||
} | ||
|
||
async fn queue_handler( | ||
Path(repo_name): Path<String>, | ||
State(state): State<ServerStateRef>, | ||
) -> Result<impl IntoResponse, AppError> { | ||
let repo = match state.db.repo_by_name(&repo_name).await? { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should fetch the repo from the app state (repositories where the GitHub app is installed), not from the DB. Because right now when you open bors with an empty DB, and it is installed on repository X, it will display "repository not found", because that repo doesn't yet have any entry in the DB (it is currently only created when the tree is closed, or some other similar operation). Or, alternatively (and I think that this would be even better, because then we wouldn't need to consider the edge case where a known repo is not in the DB), when the bot starts and fetches the repositories where it is installed, it should eagerly upsert all the found repos into the DB. Could you please do that in a separate PR? Thank you! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Working on it. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I guess this can be resolved. |
||
Some(repo) => repo, | ||
None => { | ||
return Ok(( | ||
StatusCode::NOT_FOUND, | ||
format!("Repository {repo_name} not found"), | ||
) | ||
.into_response()); | ||
} | ||
}; | ||
|
||
let prs = state.db.get_nonclosed_pull_requests(&repo.name).await?; | ||
Kobzol marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
||
// TODO: add failed count | ||
let (approved_count, rolled_up_count) = prs.iter().fold((0, 0), |(approved, rolled_up), pr| { | ||
let is_approved = if pr.is_approved() { 1 } else { 0 }; | ||
let is_rolled_up = if matches!(pr.rollup, Some(RollupMode::Always)) { | ||
1 | ||
} else { | ||
0 | ||
}; | ||
(approved + is_approved, rolled_up + is_rolled_up) | ||
}); | ||
|
||
Ok(HtmlTemplate(QueueTemplate { | ||
repo_name: repo.name.name().to_string(), | ||
repo_url: format!("https://github.com/{}", repo.name), | ||
tree_state: repo.tree_state, | ||
stats: PullRequestStats { | ||
total_count: prs.len(), | ||
approved_count, | ||
rolled_up_count, | ||
}, | ||
prs, | ||
}) | ||
.into_response()) | ||
} | ||
|
||
/// Axum handler that receives a webhook and sends it to a webhook channel. | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,12 @@ | ||
<!DOCTYPE html> | ||
<html lang="en"> | ||
<head> | ||
<meta charset="UTF-8" /> | ||
<meta name="viewport" content="width=device-width, initial-scale=1.0" /> | ||
<title>{% block title %}{% endblock %}</title> | ||
{% block head %}{% endblock %} | ||
</head> | ||
<body> | ||
{% block body %}{% endblock %} | ||
</body> | ||
</html> |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine for now (keep it like it is), but I'd like to point out that the presentation of the build status should ideally be implemented in each presentation layer (currently we only have one - the website). It's possible that we might want to render it in different ways at multiple places, and probably the logic for deciding the visualization shouldn't live in the database module.
It can be done by creating a function in the website module that renders the status, or creating some helper presentation wrapper (e.g.
BuildStatusTableDisplay
) that would wrap the status and implementDisplay
itself.Implementing
Display
on the core "domain" types is a bit of an anti-pattern, even though it's quite tempting to do, and we already did it for other structs (but there we kind of abuseDisplay
to serialize the structs form/into the DB 😆).