Skip to content
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

Update: Logic added to download the last 10 runs to then compare #220

Merged
merged 1 commit into from
Feb 17, 2025

Conversation

sihammill
Copy link
Contributor

The idea is that currently only downloading the last 2 runs means that the script will break if the last 2 runs failed for some reason.

Now, we download the last 10 runs, in which we pull out the one that is currently running and a previous run that has a status of success. This will ensure a consistent ability to compare runs.

The idea is that currently only downloading the last 2 runs means that the script will break if the last 2 runs failed for some reason.

Now, we download the last 10 runs, in which we pull out the one that is currently running and a previous run that has a status of success. This will ensure a consistent ability to compare runs.
@sihammill sihammill requested a review from YYDan February 17, 2025 15:06
@YYDan
Copy link
Contributor

YYDan commented Feb 17, 2025

You should have created an issue in this repo to propose and document this change in advance, so then any discussions could have been had and we would know what needed to be reviewed - this should always be done before putting up any pull requests

Copy link
Contributor

@YYDan YYDan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LTS support should be added at some point, but I can see that this was not supported in the previous code either, so it's not a regression.

This code/approach is quite different to how Robot gets the GM installer off GitHub, as we request the list of successful runs (and pre-ordered, I am sure), whereas you're getting all results and then manually filtering to find the successful ones. So again, I suspect there is an optimisation that could be made as a future change here.

Approving because there is a fault that we need this change now in order to fix, but I think we should look to rectify the above two points asap.

(And write up a proper issue first, next time! ;) )

@YYDan YYDan merged commit 634509d into develop Feb 17, 2025
1 check failed
@YYDan YYDan deleted the si-tf_compare_dev branch February 17, 2025 16:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants