-
-
Notifications
You must be signed in to change notification settings - Fork 2.8k
Use predictable VMs for jobs in the CI #13619
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
base: main
Are you sure you want to change the base?
Conversation
b608574
to
da50728
Compare
7b97ad5
to
4d2ee07
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.
Looks good !
I'm hesitant to merge this, because we will not catch problems in the "outdated" Python versions, which pytest technically still supports. Note that the |
I think that the VM pinning part is good. I'm also uneasy about the rest. |
In general is better not to use `-latest` runners because they are very often outdated versions, causing problems. As github does not update the name of the runners very often, it is easier to use specific ones. This also prevents surprises when github is changing the runner versions as they do this gradually and -latest ones might point to different runners in different jobs. Related: tox-dev/tox#3565 Reference: https://github.com/actions/runner-images
Btw does dependabot update those? |
Having a matrix of os (oldest/newest) while technically a good thing would double the runner cost for very little value imo. I remember a single instance where the os number version mattered in a bug report (new macos release removing an old api or us using a new mac os api implicitely, not sure). I think we should settle on oldest supported or newest supported and keep one. Using the name 'latest' for 'oldest os version supported' is very counter intuitive in any case. |
I think the PR title is misleading. The current diff basically pins the runner VMs to the extent possible. It's a good idea to pin them just like software deps for better predictability. |
While I understand the desire for pinning, I believe that sticking with the
For these reasons, the automated updates and relevance provided by |
I don't think the reasoning in your PR is valid. @ssbarnea
The term 'latest' implies the most recent version. Are you perhaps misunderstanding its meaning?
Based on historical data, GitHub designates a new runner image with the '*-latest' tag within four months (typically around two months) after it becomes stable. |
@vivodi I can only guess that you do not have much experience with GHA in general. While at industry level, 'latest' has the meaning you assumed, that is not the case for GHA runners. Looking back the last decade, ~75% the time The good part is that they add new runners only every 2-3 years for each platform, which means that using fixed ones is the better choice. I hope this explains it better. In all other cases I know I would advocate for using -latest, but not for github runners. Also there is an even worse extra reason for not using them, as it means github will randomly start to use new runners without exactly telling you when. That is because they gradually migrate repositories to newer runners during a multiple months transition when they do it. You have zero control over which runner is used if you use latest and you will only have to dig a failed job to discover that it used a different runner. Your reasoning makes sense for other stuff, not for github runners. In fact the pinned one receive updates the same way as latest ones as latest ones just point to one of the pinned ones, just that you can no way of knowing which one. So, latest is no more secure, no more reproducible. Now, if we argue about using a heavily outdated and unsupported runner that is going to be retired soon, that is another case. Yep, using If you want to keep them updated automatically, try something like https://docs.renovatebot.com/modules/manager/github-actions/ |
@ssbarnea could you add a contrib change note? This change is worth exposing to the contributors, especially more infrequent ones. |
@ssbarnea I believe that as the author of this PR, you have the responsibility to do so within this PR. The pytest repository is currently using Dependabot. |
@vivodi I don't think onboarding new automations is in the scope. But regardless, this change improves predictability greatly. |
Before this PR is merged, the pytest repository’s workflows can always run on an almost up‑to‑date runner image (given that the If this PR cannot deliver automation, then in the future the pytest repository’s workflows may end up running on a deprecated runner image, posing security risks. In that case, it would be better not to merge this PR at all. |
I don't think your assesment is reasonable: it's not like nobody maintains this repo. Such updates can be happening in a fairly predictable manner. Having a stable CI is always a priority. If anything, things like dependabot and renovate are quite spammy and this annoyance is not something that can be dismissed easily. But if you can hire somebody to be responsible for security, please do so. Burdening FOSS maintainers with this is irresponsible at best. |
The author’s original intention in creating this PR was based on the belief that the
The author of this PR wants to always use the latest runner image. Moreover, runner image updates are infrequent—no more than three times a year (for Ubuntu, macOS, and Windows). I have no idea how you arrived at the conclusion that “Burdening FOSS maintainers with this is irresponsible at best.” |
The PR scope has changed since it was opened and it is a bit different now. I don't think that description accurately reflects why this is useful. I've observed statically set VM values contributing ro stability. Having a random factor like GH updating a runner at an unspecified point in type hurts traceability and makes troubleshooting difficult when this happens. |
In general is better not to use
-latest
runners because they arevery often outdated versions, causing problems. As github does not
update the name of the runners very often, it is easier to use
specific ones. This also prevents surprises when github is changing
the runner versions as they do this gradually and -latest ones might
point to different runners in different jobs.
Reference: https://github.com/actions/runner-images