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.

Sign up for GitHub

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

Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

Fix potential github action smells #2684

Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
f136145
Fix gha smells:
ceddy4395 May 24, 2024
1bde077
Include suggestions from PR:
ceddy4395 May 27, 2024
2b20b0b
Fix gha smells in other workflows as well:
ceddy4395 May 27, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 9 additions & 8 deletions .github/workflows/build.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,8 +16,8 @@ jobs:
- java: 21
# Disable Enforcer check which (intentionally) prevents using JDK 21 for building
extra-mvn-args: -Denforcer.fail=false
runs-on: ubuntu-latest

runs-on: ubuntu-22.04
timeout-minutes: 3
steps:
- uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4
- name: "Set up JDK ${{ matrix.java }}"
Expand All @@ -32,8 +32,8 @@ jobs:

native-image-test:
name: "GraalVM Native Image test"
runs-on: ubuntu-latest

runs-on: ubuntu-22.04
timeout-minutes: 3
steps:
- uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4
- name: "Set up GraalVM"
Expand All @@ -51,8 +51,8 @@ jobs:

verify-reproducible-build:
name: "Verify reproducible build"
runs-on: ubuntu-latest

runs-on: ubuntu-22.04
timeout-minutes: 3
steps:
- uses: actions/checkout@0ad4b8fadaa221de15dcec353f45205ec38ea70b # v4.1.4
- name: "Set up JDK 17"
Expand All @@ -65,11 +65,12 @@ jobs:
- name: "Verify no plugin issues"
run: mvn artifact:check-buildplan --batch-mode --no-transfer-progress

- name: "Verify reproducible build"
- name: "Do clean install of dependencies"
Copy link
Collaborator

@Marcono1234 Marcono1234 May 24, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

This should probably rather say "Build project" or similar.

In Maven install is a lifecycle phase, and it includes all previous phases and therefore performs a full build (except for deployment), see https://maven.apache.org/guides/introduction/introduction-to-the-lifecycle.html#a-build-lifecycle-is-made-up-of-phases

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for the suggestion, changed it!

# See https://maven.apache.org/guides/mini/guide-reproducible-builds.html#how-to-test-my-maven-build-reproducibility
Copy link
Collaborator

Choose a reason for hiding this comment

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

Might be good to move this # See ... comment above the name: "Do clean ... since it applies to both steps now and not just the mvn clean install.

Copy link
Author

Choose a reason for hiding this comment

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

Ah oke yes makes sense. I've updated this.

run: |
mvn clean install --batch-mode --no-transfer-progress -Dproguard.skip -DskipTests
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could simplify this by moving the command in the same line:

Suggested change
run: |
mvn clean install --batch-mode --no-transfer-progress -Dproguard.skip -DskipTests
run: mvn clean install --batch-mode --no-transfer-progress -Dproguard.skip -DskipTests

Copy link
Author

Choose a reason for hiding this comment

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

I've fixed this. Also for the other files I've made the same change to keep everything consistent.

# Run with `-Dbuildinfo.attach=false`; otherwise `artifact:compare` fails because it creates a `.buildinfo` file which
# erroneously references the existing `.buildinfo` file (respectively because it is overwriting it, a file with size 0)
# See https://issues.apache.org/jira/browse/MARTIFACT-57
Copy link
Collaborator

Choose a reason for hiding this comment

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

Since these # Run with ... comments are only relevant for the mvn command below, it would be good to move them down (and adjust their indentation), so that they are between the name and the run below.

Copy link
Author

Choose a reason for hiding this comment

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

Ah yes moved them

mvn clean verify artifact:compare --batch-mode --no-transfer-progress -Dproguard.skip -DskipTests -Dbuildinfo.attach=false
- name: "Verify reproducible build"
run: mvn clean verify artifact:compare --batch-mode --no-transfer-progress -Dproguard.skip -DskipTests -Dbuildinfo.attach=false