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 #3770

Merged
merged 1 commit into from
May 28, 2024

Conversation

ceddy4395
Copy link
Contributor

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

  • Use cache parameter instead of cache option
    • Using the cache option from the setup/java is easier to configure and thus less prone to mistakes. Furthermore, it makes the workflow smaller and easier to read and understand.
  • Avoid jobs without timeouts
    • Jobs without a timeout can cause runners to be occupied when some process falls into an infinite loop or has multiple retries to perform a certain task, which will inevitably fail. Furthermore, it is also useful to be notified when a test-suite all of a sudden takes a lot longer than it used to, considering keeping tests fast is a good practice.

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

Copy link
Member

@codefromthecrypt codefromthecrypt left a comment

Choose a reason for hiding this comment

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

looks good to me on green. thanks!

(you'll notice we have many repos with the same actions, the most production are zipkin-reporter-java, brave, zipkin-dependencies, zipkin-aws and zipkin-gcp if bored ;))

@ceddy4395
Copy link
Contributor Author

Thank you for the approval!
I'll have a look if I have some time to spare! :)

I noticed the lint job is failing because of some markdown links giving a 404? Would you like to have that fixed in this PR or in a different one?

@codefromthecrypt
Copy link
Member

maybe you can rebase this and possibly it will sort out those links? I think a recent change fixed it.

- Use cache parameter instead of cache option
- Avoid jobs without timeouts
@ceddy4395
Copy link
Contributor Author

@codefromthecrypt Ah yes that did indeed fix the problems! Thank you

@codefromthecrypt codefromthecrypt merged commit 2f0a26f into openzipkin:master May 28, 2024
14 checks passed
@codefromthecrypt
Copy link
Member

thanks for the help and teaching us new tricks @ceddy4395!

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