Fix potential github action smells #28909
Conversation
- Prevent running issue/PR actions on forks - Use fixed version for runs-on argument - Use 'if' for upload-artifact action - Avoid deploying jobs on forks - Avoid installing packages without version
| @@ -193,7 +194,7 @@ jobs: | |||
| # Build the source distribution under Linux | |||
| build_sdist: | |||
| name: Source distribution | |||
| runs-on: ubuntu-latest | |||
| runs-on: ubuntu-22.04 | |||
There was a problem hiding this comment.
What is the reason for this change?
There was a problem hiding this comment.
It is a change I've come across during my study, therefore I'm interested to know if other developers would agree if locking the runs-on version to a specific version.
In terms of my reasoning why it would be better: If Github at any point in time moves to the next version of Ubuntu you will automatically also be moved over on your CI. If, in the unlikely case that there is a (security?) bug in this version of ubuntu you could experience negative side effects because of this.
There was a problem hiding this comment.
Security vulnerability are likely to be addressed in future bug fixes. In addition, this is likely that the vulnerabilities exist in previous version (if they are also LTS version then we should get support as well).
Basically, in the past we always have been required to manually bump the version and sometimes we don't really see the deprecation of the image. I think that I'm more convinced that using the latest in this case is less headache.
There was a problem hiding this comment.
ping @lesteve @ogrisel @thomasjpfan
There was a problem hiding this comment.
I think whether to pin the version of the image or not depends a bit on what the workflow is doing.
In this case I think the benefit that Guillaume described (automatic updates, less manual version bumping) are a good thing. This is because the work that happens in the workflow should reasonably "just work" on all reasonable Ubuntu base images.
.github/workflows/check-sdist.yml
Outdated
| @@ -19,13 +19,13 @@ jobs: | |||
| # scipy and cython are required to build sdist | |||
| run: | | |||
| python -m pip install --upgrade pip | |||
| pip install check-sdist | |||
| pip install check-sdist==0.1.4 | |||
There was a problem hiding this comment.
I don't think that we want to pin this version. We for sure want to benefit from future development and bug fixes.
There was a problem hiding this comment.
Ah okee I understand, thank you for the feedback! I've taken it out :)
| @@ -178,12 +178,13 @@ jobs: | |||
|
|
|||
| - name: Store artifacts | |||
| uses: actions/upload-artifact@v3 | |||
| if: ${{success() && github.repository == 'scikit-learn/scikit-learn'}} | |||
There was a problem hiding this comment.
Why add success() to the if here? I don't know if it was missing in the past or if it was left off on purpose.
There was a problem hiding this comment.
In case of build or test failure in the previous step, that "Store artifacts" step would not have been executed, right?
|
Thanks for investigating this. I think people will ask a few questions about the changes. The trouble with changes to workflows like this is that things that look innocent can have big unexpected consequences. From harmless things like "workflow isn't correct anymore" to "subtly introduce a vulnerability" - this makes people ask probing questions. This is a good thing IMHO but could be misunderstood as "grumpy old people" behaviour. So I thought it was worth mentioning it. |
@ceddy4395 I want to double down @betatim's comment. I'm getting old but I'm not grumpy :). Just probing potential side effect. |
|
I don't understand the motivation to not run the workflows on forks by manually protecting everything with the Doing so would:
but I don't really see much security benefit (if any). Do you have examples of particular vulnerabilities that are typically mitigated by this? Similarly for storing artifacts actions: they already require to have configured secret credentials. I don't see how adding this condition will mitigate any kind of problem. |
Historically, this was done to not use up resources on forks. On the Github Free Plan, there is a 20 job limit on GitHub hosted runners. If a fork's jobs are running on a user's account, it may take longer for CI jobs to run on the user's other repos. Now a days, GitHub will turn off GitHub actions on forks after sometime and new forks will not have GitHub Actions enabled by default. So it's less of an issue now. |
|
I am going to close this PR, sorry if this sounds a bit harsh but I think this is below the "useful enough to warranty maintainers time" threshold. @ceddy4395 personally I am growing more and more skeptical about people opening similar PRs in plenty of repositories like you did. Since this PR is about smells I would go as far as calling this behaviour a bad smell 😉. It seems like you are doing it with the best intentions in mind for your master thesis but still ... |
Reference Issues/PRs
What does this implement/fix? Explain your changes.
Hey! 🙂
I want to contribute the following changes to your workflow:
Any other comments?
These changes are part of a research Study at TU Delft looking at GitHub Action Smells. Find out more