Store duration in database

[bncer]

Related to #486

Note:

• running_time from BorsRepositoryEvent::WorkflowCompleted::WorkflowRunCompleted, which calculates duration
• Currently in this PR try builds written to the duration column of the build table for possible future use. Try build duration records should not effect performance, we will filter out only successfully completed auto builds.

[Kobzol]

Hi, thank you for the PR! Could you also add code that loads the duration from the DB into the build model? Just so that we can check that the Interval type can be correctly deserialized into Rust.

[bncer]

Sorry. I'm busy this week. I will do as soon as I have time

[bncer]

Hi!
When I try to work on expected time I face the same issue.

Now I notice some interesting things on:
cargo sqlx prepare -- --all-targets
Sqlx changes my .sqlx/some_hash.json files from nullable false to true
example from 367bf2de86a8a92b56b2be296cef119a7835ab5fd09326265a4a1403d0d6ad19.json

    "nullable": [
      true,
      true,
      true,
      true,
      true,
      true,
      null,
      true,
      true,
      true,
      true,
      true,
      true,
      true,
      true,
      null
    ]
  },
  "hash": "367bf2de86a8a92b56b2be296cef119a7835ab5fd09326265a4a1403d0d6ad19"

The same way f697693a6cbccdf9b2557842a1ff87959d6e85012dba5e9d7159541ef317b691.json changed nullable to true

Only common pattern where I see the error is query with CTE(Common Table Expression).
I'm digging what can cause the issue. Maybe sqlx, database rows, CTE ...

[Kobzol]

Interesting. Are you using the same Postgres version that we have in docker-compose.yml?

[bncer]

Yes, I haven't changed anything related to database.
Is putting "!" mark on not nullable columns of query an option?

[Kobzol]

I guess so, but I would also like to find out what differs for you. If you use the same Postgres, and the same sqlx version, and the same code, it should end up with the same result 😆 Does it also happen on the main branch? What's your OS?

[bncer]

Yes, currently on main branch
OS spec:
NAME="Fedora Linux"
VERSION="41 (Workstation Edition)"
RELEASE_TYPE=stable

Also I discovered sqlx prepare triggers cargo check and errors occur and seeing errors sqlx changes *.json chache files:

error[E0277]: the trait bound `DateTime<Utc>: From<std::option::Option<DateTime<Utc>>>` is not satisfied
   --> src/database/operations.rs:278:22
    |
278 |           let result = sqlx::query_as!(
    |  ______________________^
279 | |             PullRequestModel,
...   |
320 | |             base_branch,
321 | |         )
    | |_________^ the trait `From<std::option::Option<DateTime<Utc>>>` is not implemented for `DateTime<Utc>`
    |
    = help: the following other types implement trait `From<T>`:
              `DateTime<Utc>` implements `From<DateTime<FixedOffset>>`
              `DateTime<Utc>` implements `From<DateTime<Local>>`
              `DateTime<Utc>` implements `From<SystemTime>`
    = note: required for `std::option::Option<DateTime<Utc>>` to implement `Into<DateTime<Utc>>`
    = note: this error originates in the macro `$crate::sqlx_macros::expand_query` which comes from the expansion of the macro `sqlx::query_as` (in Nightly builds, run with -
Z macro-backtrace for more info)

For more information about this error, try `rustc --explain E0277`.
error: could not compile `bors` (lib) due to 14 previous errors
warning: build failed, waiting for other jobs to finish...
error: could not compile `bors` (lib test) due to 14 previous errors
error: `cargo check` failed with status: exit status: 101

[bncer]

I also tried to look into marcos sqlx::query_as! with cargo expand but couldn't do it properly so far.

[Kobzol]

Have you tried removing the target directory, and ensuring that there is nothing extra in .sqlx? There can be uncommitted files sometimes. rm -rf target && rm -rf .sqlx && git reset --hard <upstream>/main.

[bncer]

This commands rm -rf target && rm -rf .sqlx && git reset --hard <upstream>/main didn't help.

The error was gone after I deleted bors database in docker container. Somehow local database content affect Option in code, even though sqlx used as offline if I understand correctly what offline means.
Anyway I restored my database with old backup from last year :)

[Kobzol]

Ah, it that happens again, just do cargo sqlx database reset :)

[bncer]

WIP. Do not review immediately, please.
It was a bit tricky to convert database's interval to rust's duration.
Maybe I shouldn't touch the extension of update_build_{status, duration}, but I tried to show you my version of solution.
I will wrap up everything and finish as soon as possible.

[Kobzol]

Thanks! Let me know when I can take a look. (Also, no hurry, I have a lot of other things to fix/improve in bors in the meantime xD)

[Kobzol]

I think that the status doesn't have to be optional - the only time we update the duration, we also set the status, right? And regarding the SQL query, I think that COALESCE is a good solution.

Btw I think that some rebase went wrong, and the get_merge_queue_prs function has reappeared in your PR (it has since been deleted).

[Kobzol]

Thank you! Left some comments to simplify the Interval operations and about the actual build duration logic.

[Kobzol]

That being said, what you have should already work for the common case! And I don't want to be too nitpicky here. So if you want, we can merge the PR and I can then make the changes to compute the running time from the set of workflows, as I expect that it will require also some changes to the testing infrastructure.

[bncer]

Yes, I want this PR to be merged. I will continue on representing expected time

[bncer]

Also I have to point out update_build_check_run_id can also be included to update_build_column

[Kobzol]

Ok! I'll merge this and make some improvements (refactor the build DB functions, try to load the duration from workflows, write some tests), and in the meantime you can take a look at representing the expected duration. It's mostly a UX thing, I'm not sure what's the best way to do it - reuse the vertical yellow bar? Or create a new horizontal progress bar? Or maybe ditch the "pending" string and just show a progress bar instead of it? So many options! :)

Thank you, great work!