-
Notifications
You must be signed in to change notification settings - Fork 20
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
feat: added min duaration checking in RepositoryConfig #164
Conversation
Hi! This is a good start, but the bot should actually use this value, and check that the CI workflow has not in fact completed in a shorter time than this. |
I will check on that. So i think we need to write a test and ensure that min_ci_time is maintained or not right? |
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.
Thanks for the PR, it's a good start. Left one comment about the actual logic that should happen on failure.
We should also add some basic tests for this. You can check the existing tests to see how we define them.
e7c106e
to
380d43b
Compare
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.
I left a few more comments. You can find tests e.g. at the bottom of the workflow.rs
file.
To run tests locally, you'll need to setup a DB, which I tried to describe in docs/development.md
. Let me know if you have any questions about that.
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.
Sorry for the long review time, I was (and still am) very busy.
Left a few comments, and we still need tests. If you don't want to work on them, it's fine, I can add them later.
df9a8b9
to
c0228b8
Compare
@Kobzol I have made the changes can you review them and can you give me a idea of tests to write regarding this. |
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.
I implemented a simple test, that also changed internal logic for determining if a check suite was failed or not.
In the future, we should also generalize the workflow status to have explicit information about running for too short, as the error message in this case is confusing. But let's not do that in this PR.
cf5c3bf
to
403e140
Compare
It seems like your forcep ush removed my commit, but that doesn't matter, I will add the tests in another PR. Could you please squash the commits? I will merge this PR after that. Thank you! |
Changes to be committed: modified: src/bors/event.rs modified: src/bors/handlers/workflow.rs modified: src/config.rs modified: src/github/webhook.rs
403e140
to
34b52b3
Compare
Thank you! :) |
Fixes: #84
I have added a serilazed min_ci_duartion field which takes care that atleast 60 seconds of testing is being done.