-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix potential github action smells #8836
base: master
Are you sure you want to change the base?
Conversation
- Avoid uploading artifacts on forks - Avoid starting new workflow whilst the previous one is still running - Run tests on multiple OS's - Stop running workflows when there is a newer commit in branch
Thank you for your pull request and welcome to our community! To contribute, please sign the Oracle Contributor Agreement (OCA).
To sign the OCA, please create an Oracle account and sign the OCA in Oracle's Contributor Agreement Application. When signing the OCA, please provide your GitHub username. After signing the OCA and getting an OCA approval from Oracle, this PR will be automatically updated. If you are an Oracle employee, please make sure that you are a member of the main Oracle GitHub organization, and your membership in this organization is public. |
Thank you for signing the OCA. |
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.
Thanks for the contribution. it was a conscious decision to keep the number of OS version to a minimum, and windows-2019
won't even work. The concurrency
policy may be useful to have. Please take a look at my comments.
.github/workflows/quarkus.yml
Outdated
@@ -51,6 +51,10 @@ on: | |||
- cron: '0 3 * * *' | |||
workflow_dispatch: | |||
|
|||
concurrency: | |||
group: ${{github.workflow}} - ${{github.ref}} |
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.
Why - ${{github.ref}}
here and not in micronaut.yml
? There are also other workflows that may need a concurrency
rule such as spring.yml
.
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.
I've added the ${{github.ref}}
because it can also be triggered by pull requests. Therefore, I was assuming that if there are two pr's running this workflow, one should not cancel the other.
If this is not the case, the ${{github.ref}}
can be removed.
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.
In main.yml
, we've got:
group: "workflow = ${{ github.workflow }}, ref = ${{ github.event.ref }}, pr = ${{ github.event.pull_request.id }}"
Any reason not to use this consistently? My point was also about being consistent across all other workflows (e.g., micronaut.yml and spring.yml)
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.
Ahh I see, sorry for the misunderstanding.
I've changed it so that it is consistent with the main.yml
. Same for the reachabilit-metadata.yml
, spring.yml
and micronaut.yml
.
I did not change the cdt-inspect.yml
because this is only triggered through a schedule.
.github/workflows/main.yml
Outdated
@@ -91,7 +91,7 @@ permissions: | |||
jobs: | |||
build-graalvm-linux: | |||
name: /${{ matrix.env.PRIMARY }} ${{ matrix.env.GATE_TAGS }} JDK${{ matrix.env.JDK_VERSION }} | |||
runs-on: ubuntu-20.04 | |||
runs-on: ubuntu-22.04 |
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.
Can we keep ubuntu-20.04
for now?
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.
Yes of course, mistake on my side!
Out of curiosity, any specific reason not to use 22.04?
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.
Yes: we want to test with the oldest glibc available (ubuntu-20.04 is the oldest ubuntu currently supported by GHA), so that we know things are still working there. The moment the glibc version gets bumped, we will know because the builds will fail and then will we move to 22.04. The same is true for any other dev and non-dev dependency.
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.
I see, thanks for the clarification 馃槃
bddb8f0
to
5c5fe5f
Compare
Make all groups names be in the format of: workflow = ${{ github.workflow }}, ref = ${{ github.event.ref }}, pr = ${{ github.event.pull_request.id }}
Hey! 馃檪
I want to contribute the following changes to your workflow:
(These changes are part of a research Study at TU Delft looking at GitHub Action Smells. Find out more)