Add queue page #291
Add queue page #291
Conversation
|
You'll notice a lack of styling because I figured there's no point in adding it as I plan to mock up some slight redesign iterations, and for now, the logic is more important. |
templates/index.html
Outdated
| bors is configured to use. (e.g. for the Rust project this is <code>@bors</code>) | ||
| Note that bors will only recognize comments in <i>open</i> PRs. | ||
| bors is configured to use. (e.g. for the Rust project this is | ||
| <code>@bors</code>) Note that bors will only recognize comments in |
There was a problem hiding this comment.
This is not currently the case with bors, right? I think it will happily run commands even in closed/merged PRs right now.
There was a problem hiding this comment.
Ahh yeah, right now it will.
Homu deletes PRs from in-memory state and the database once merged and then skips webhook calls for these since they no longer exist to it.
I'm assuming we should do something similar? In which case that statement will become true but right now it's not.
There was a problem hiding this comment.
I'm not sure if we really need to delete PRs from the database, but in the long term we might implement something like that to reduce the DB storage, yeah (although I suspect it will be so small anyway that it doesn't really matter).
| @@ -251,6 +251,18 @@ pub enum BuildStatus { | |||
| Timeouted, | |||
| } | |||
|
|
|||
| impl Display for BuildStatus { | |||
There was a problem hiding this comment.
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 implement Display 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 abuse Display to serialize the structs form/into the DB 😆).
| 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.
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.
I guess this can be resolved.
|
Let me know if you want to merge #291 (comment) first, or if we should first merge this, doesn't really matter. If you want to merge this PR first, please squash the commits. |
|
Will merge that PR first then come back to this. |
Sakib25800
force-pushed
the
add-queue-site
branch
from
2b3260e to
9e1d19f
Compare
|
Just realised that they're two completely separate PRs now. I was going to fetch repos from state rather than the database but realised that we need Squashed. |
- Add queue table - Add queue statistics - Redirect homepage to queue page - Replace original homepage with /help page
Sakib25800
force-pushed
the
add-queue-site
branch
from
9e1d19f to
bb07e36
Compare
This PR does the following:
/queue/<repo_name>page/queue/rust/helppage