Update CI to maitain the artifacts and add a upper job run limit #535
Conversation
MuhammadHammad001
changed the title
| @@ -27,6 +27,8 @@ jobs: | |||
| ACT-sail-spike: | |||
| name: ACT-sail-spike (RV${{ matrix.xlen }}) | |||
| runs-on: ubuntu-22.04 | |||
| # Set a 15-minute time limit for this job | |||
There was a problem hiding this comment.
As we add other extensions to CI (like F) the duration of the CI will increase significantly and likely take more than 15 minutes. I wonder if we’re better off just letting it fail by timing out. Or at least setting a higher timeout time around 1 hour.
There was a problem hiding this comment.
Yep, I updated the time limit to 30 minutes but surely we can update it to 60 minutes as well.
| name: riscof-test-report-rv${{ matrix.xlen }} | ||
| path: /home/runner/work/riscv-arch-test/riscv-arch-test/riscof-plugins/rv${{ matrix.xlen }}/riscof_work/ | ||
| if-no-files-found: warn | ||
| retention-days: 3 |
There was a problem hiding this comment.
Many of these are the defaults already so probably aren’t necessary to specify. Also, is there a reason to override the default, longer retention period? I believe the files will get deleted automatically if the repo runs out of space. 3 days seems kind of short as sometimes people don’t come back immediately to fix PRs.
There was a problem hiding this comment.
Actually, I thought it would be better to retain the artifacts only for 3 days since the developers usually need them in case of a failure and to save the space for other PRs as well. But, the default is 90 days, please let me know if you want me to change it to default and regarding the other default values, yes, I added include-hidden-files option in case someone wants to change in future but in our case, it's currently redundant, so I will remove that as well.
There was a problem hiding this comment.
If it runs out of space it will delete the oldest ones automatically, so deleting them earlier doesn't really save space for other PRs. Because of that I don't really see a reason to force them to be deleted early. @UmerShahidengr any opinion on this?
|
Looks like this was merged as I submitted these comments. I can open a follow up PR to address. @UmerShahidengr |
|
@UmerShahidengr More importantly, this won't currently upload the artifacts when there is a failure, which is when they are most likely to be needed. |
Description
This PR is meant to update the CI such that:
Related Issues