Tree Closing and Opening #211

Rhythm1710 · 2025-03-05T22:06:14Z

Issue #207

Kobzol

Thank you! Left a bunch of comments.

Once we have the repository in the DB, we should link PRs to repositories through foreign keys, so that we can easily join them in queries (we will need to figure out if a repo for a given PR is closed or not, and get PRs belonging to a given repo). This doesn't need to happen in this PR, but probably we'll want to change the repository field of PRs to become a FK to the repository table, and then always try to insert a repo into the DB when creating new PRs.

Kobzol · 2025-03-06T08:17:50Z

migrations/20250304172534_add_repo_model.down.sql

@@ -0,0 +1,8 @@
+-- Add down migration script here
+CREATE TABLE repo_model (


Just repository, to be consistent with the rest of the tables.

Kobzol · 2025-03-06T08:19:10Z

migrations/20250304172534_add_repo_model.down.sql

+-- Add down migration script here
+CREATE TABLE repo_model (
+    id SERIAL PRIMARY KEY,
+    repo TEXT NOT NULL,


repo -> name

Kobzol · 2025-03-06T08:19:55Z

migrations/20250304172534_add_repo_model.down.sql

+CREATE TABLE repo_model (
+    id SERIAL PRIMARY KEY,
+    repo TEXT NOT NULL,
+    treeclosed TEXT NOT NULL CHECK (treeclosed IN ('open', 'closed')),


Homu stored a numeric value P that states "closed for PRs below priority P". So I would store this column as treeclosed INT NULL, where NULL means "not closed" and a non-null value means a closed tree with the priority P.

Kobzol · 2025-03-06T08:23:36Z

src/bors/command/parser.rs

@@ -321,6 +323,30 @@ fn parse_priority_arg<'a>(parts: &[CommandPart<'a>]) -> Result<Option<u32>, Comm
    Ok(priority)
 }

+fn parse_tree_open<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> {


Let's use the original homu commands, so treeclosed- for opening a tree.

Kobzol · 2025-03-06T08:25:10Z

src/bors/event.rs

@@ -18,6 +18,8 @@ pub enum BorsRepositoryEvent {
    /// A check suite has been completed, either as a workflow run on Github Actions, or as a
    /// workflow from some external CI system.
    CheckSuiteCompleted(CheckSuiteCompleted),
+    /// When a repository's tree state changes (open/closed)
+    TreeStateChanged(TreeStateChanged),


I don't think that we need any event for this? It should be enough to just handle the comment and store information about closed/opened repo in the DB.

Kobzol · 2025-03-06T08:26:11Z

src/bors/handlers/queue.rs

+}
+
+/// Process the queue of pull requests for each repository
+pub async fn process_queue(


This seems like it implements a basic merge functionality or something? Let's not do that in this PR :) Just store the information about the tree being closed/open in the DB and that's it.

Will we need this in future? I can just comment it out and will be helpful in the future

I don't know how the merge functionality will look like, that will require some up-front planning, design and discussions. So let's remove this for now, it's not needed for basic tree open/close functionality.

Ok, Removing it

Kobzol

Left a few comments :)

Kobzol · 2025-03-06T16:36:13Z

migrations/20250304172534_add_repository_model.down.sql

+CREATE TABLE repository (
+    id SERIAL PRIMARY KEY,
+    name TEXT NOT NULL UNIQUE,
+    treeclosed INT NULL,


Suggested change

treeclosed INT NULL,

treeclosed BOOLEAN NULL,

Postgres supports booleans directly :)

In this dont we have to store priority numeric value of treeclosed?

Kobzol · 2025-03-06T16:48:22Z

src/bors/command/parser.rs

@@ -112,6 +119,20 @@ fn parse_parts(input: &str) -> Result<Vec<CommandPart>, CommandParseError> {
            break;
        }

+        // Special handling for r- command
+        if item == "r-" {


I would prefer to avoid special cases here. We can parse the two commands in a much simpler way, e.g.:

fn parse_tree_open<'a>(command: &'a str, _parts: &[CommandPart<'a>]) -> ParseResult<'a> { if command != "treeclosed-" { return None; } Some(Ok(BorsCommand::TreeOpen)) } fn parse_tree_closed<'a>(command: &'a str, parts: &[CommandPart<'a>]) -> ParseResult<'a> { if command != "treeclosed" { return None; } match parts { [CommandPart::KeyValue { value, .. }] => match validate_priority(value) { Ok(priority) => Some(Ok(BorsCommand::TreeClosed(priority))), Err(e) => Some(Err(e)), }, _ => Some(Err(CommandParseError::MissingArgValue { arg: "treeclosed", })), } }

My bad! I will change this

Kobzol · 2025-03-06T16:49:20Z

src/bors/handlers/help.rs

@@ -65,6 +67,12 @@ fn get_command_help(command: BorsCommand) -> String {
        BorsCommand::TryCancel => {
            "`try cancel`: Cancel a running try build"
        }
+        BorsCommand::TreeOpen => {
+            "`tree open`: Open the repository tree for merging"


Suggested change

"`tree open`: Open the repository tree for merging"

"`treeclosed-`: Open the repository tree for merging"

Changed this

Kobzol · 2025-03-06T16:51:57Z

src/config.rs

@@ -21,6 +21,8 @@ pub struct RepositoryConfig {
    pub labels: HashMap<LabelTrigger, Vec<LabelModification>>,
    #[serde(default, deserialize_with = "deserialize_duration_from_secs_opt")]
    pub min_ci_time: Option<Duration>,
+    #[serde(default)]
+    pub treeclosed: Option<u32>,


What does this do?

In this we were setting tree closed, changing that to tree open by default

This is a struct that represents a configuration of bors per each repository. Why should it mention treeclosed?

Kobzol · 2025-03-06T16:53:23Z

src/database/operations.rs

+    let row = sqlx::query_as!(
+        RepoModel,
+        r#"
+        SELECT id, name, COALESCE(treeclosed, 0) as "treeclosed: TreeState", treeclosed_src, created_at


The COALESCE shouldn't be needed, since we're loading the treestate as Option<i32>, right? 🤔

Yes it was not needed, fixed this as well.

Kobzol · 2025-03-07T16:22:16Z

You have a merge commit here. You should rebase onto master instead. Something like this:

git fetch origin
git rebase origin/main
git push --force

Kobzol reviewed Mar 6, 2025

View reviewed changes

Rhythm1710 closed this Mar 8, 2025

Rhythm1710 reopened this Mar 8, 2025

Rhythm1710 force-pushed the main branch from fa9f556 to 34bcc6b Compare March 8, 2025 19:36

Rhythm1710 closed this Mar 8, 2025

Rhythm1710 force-pushed the main branch from 34bcc6b to a67338b Compare March 8, 2025 19:45

Rhythm1710 mentioned this pull request Mar 8, 2025

Tree Closing and Opening #223

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Tree Closing and Opening #211

Tree Closing and Opening #211

Rhythm1710 commented Mar 5, 2025 •

edited

Loading

Kobzol left a comment

Kobzol Mar 6, 2025

Kobzol Mar 6, 2025

Kobzol Mar 6, 2025

Kobzol Mar 6, 2025

Kobzol Mar 6, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol left a comment

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025

Kobzol Mar 7, 2025

Kobzol Mar 6, 2025

Rhythm1710 Mar 6, 2025 •

edited

Loading

Kobzol commented Mar 7, 2025

		@@ -0,0 +1,8 @@
		-- Add down migration script here
		CREATE TABLE repo_model (

	"`tree open`: Open the repository tree for merging"
	"`treeclosed-`: Open the repository tree for merging"

Tree Closing and Opening #211

Tree Closing and Opening #211

Conversation

Rhythm1710 commented Mar 5, 2025 • edited Loading

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kobzol left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Rhythm1710 Mar 6, 2025 • edited Loading

Choose a reason for hiding this comment

Kobzol commented Mar 7, 2025

Rhythm1710 commented Mar 5, 2025 •

edited

Loading

Rhythm1710 Mar 6, 2025 •

edited

Loading