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

Fix potential github action smells #1156

Merged
merged 1 commit into from
May 29, 2024

Conversation

ceddy4395
Copy link
Contributor

Hey! 🙂
I've made the following changes to the workflow:

  • Avoid installing packages without version
    • When installing a package without a version, the latest version will always be installed, even when braking changes have been released. To avoid this causing failing pipelines a version should be specified which is known to be compatible with the code base.
  • Define permissions for workflows with external actions
    • Permissions should be used when running actions written by other developers because there may be security leaks exposed through these actions.
  • Avoid running CI related actions when no source code has changed
    • Running workflows which build or test certain parts of the code base without these parts being changed is useless as nothing new will be checked or created.
  • Stop running workflows when there is a newer commit in PR
    • When a new commit is added to a PR, it is not needed any more to test the old version of the PR because this does not tell us anything about the current state of the PR. Therefore, for efficiency, we can cancel it and only test the latest version.

(These changes are part of a research Study at TU Delft looking at GitHub Action Smells. Find out more)

@ceddy4395 ceddy4395 requested a review from ATheorell as a code owner May 22, 2024 14:18
Copy link
Collaborator

@ATheorell ATheorell left a comment

Choose a reason for hiding this comment

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

This is an appreciated contribution!
Note that you have to remove a trailing white space to pass the last check.
Please answer the specific question.

@@ -23,7 +31,7 @@ jobs:
cache: 'pip' #Note that pip is for the tox level. Poetry is still used for installing the specific environments (tox.ini)

- name: Install tox
run: pip install tox
run: pip install tox==4.15.0
Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be nice if you explained why this change is needed.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The idea is to keep the pipeline as consistent as possible between runs, and to make sure it is the developer's conscious decision to use the next version during the workflow.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I agree on consistency, I was curious whether there was a reason for this specific version.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah oke, this is just the current version, that's why.

@ceddy4395
Copy link
Contributor Author

@ATheorell Thank you for the feedback. I've removed the trailing white spaces and replied to your comments.

@ATheorell
Copy link
Collaborator

Thanks for the explanation! pre-commit doesn't seem to agree about the trailing whitespace.

- Avoid installing packages without version
- Define permissions for workflows with external actions
- Avoid running CI related actions when no source code has changed
- Stop running workflows when there is a newer commit in PR
@ceddy4395
Copy link
Contributor Author

Ah yes I see I forgot one by accident! This is now fixed as well :)

@ATheorell ATheorell merged commit 5f9d7ac into gpt-engineer-org:main May 29, 2024
4 checks passed
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