Fix potential github action smells #2684

ceddy4395 · 2024-05-24T11:40:10Z

Purpose

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

Use fixed version for runs-on argument
- Using an explicit version for runs-on makes the developers more conscious about on what OS version they are testing/supporting. Furthermore, if breaking changes are introduced in the next version, they will not automatically break the pipeline.
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.
Steps should only perform a single command
- Making sure every step has a single responsibility makes it easier to understand the workflow and, in case a workflow fails, it makes it clearer where and why it failed.

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

Description

Checklist

New code follows the Google Java Style Guide
This is automatically checked by mvn verify, but can also be checked on its own using mvn spotless:check.
Style violations can be fixed using mvn spotless:apply; this can be done in a separate commit to verify that it did not cause undesired changes.
If necessary, new public API validates arguments, for example rejects null
New public API has Javadoc
- Javadoc uses @since $next-version$
  ( $next-version$ is a special placeholder which is automatically replaced during release)
If necessary, new unit tests have been added
- Assertions in unit tests use Truth, see existing tests
- No JUnit 3 features are used (such as extending class TestCase)
- If this pull request fixes a bug, a new test was added for a situation which failed previously and is now fixed
mvn clean verify javadoc:jar passes without errors

google-cla · 2024-05-24T11:40:14Z

Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA).

View this failed invocation of the CLA check for more information.

For the most up to date status, view the checks section at the bottom of the pull request.

- Use fixed version for runs-on argument - Avoid jobs without timeouts - Steps should only perform a single command

Marcono1234

Thanks!

To me this looks reasonable. I have added a few review comments but they are mainly about formatting.

A few general points though:

The hardcoded Ubuntu version might add some (very small) maintenance overhead due to having to remember to update it in the future
But I assume that is acceptable.
The timeouts might be a bit too low, especially since the run duration probably depends on external factors we cannot control
- "GraalVM Native Image test" should maybe have 10 minutes as timeout; for example here is a run where it took 4 minutes: https://github.com/google/gson/actions/runs/8910979694/job/24471354114
- the other jobs should probably have 5 minutes as timeout, to be safe
It probably makes sense to apply these changes also to the other workflows under .github/workflows
Would you like to do that as well?

@eamonnmcmanus, what do you think?

Marcono1234 · 2024-05-24T22:23:47Z

.github/workflows/build.yml

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


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

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

Marcono1234 · 2024-05-24T22:24:36Z

.github/workflows/build.yml

          # 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


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.

Ah yes moved them

Marcono1234 · 2024-05-24T22:25:42Z

.github/workflows/build.yml

@@ -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"
        # See https://maven.apache.org/guides/mini/guide-reproducible-builds.html#how-to-test-my-maven-build-reproducibility


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.

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

Marcono1234 · 2024-05-24T22:38:45Z

.github/workflows/build.yml

@@ -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"


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

Thank you for the suggestion, changed it!

- Increase timeout - Change name for build step - Move documentation closer to run.

- Use fixed version for runs-on argument - Avoid jobs without timeouts - Steps should only perform a single command

ceddy4395 · 2024-05-27T12:28:08Z

@Marcono1234 Thank you for the review! I've made the changes you've suggested, including bumping the timeouts.

I've also made the same fixes to the other workflow files!

eamonnmcmanus · 2024-05-29T14:03:06Z

Thank you for doing this! Overall it looks good, but I'm not entirely convinced by this:

Use fixed version for runs-on argument

Using an explicit version for runs-on makes the developers more conscious about on what OS version they are testing/supporting. Furthermore, if breaking changes are introduced in the next version, they will not automatically break the pipeline.

Would Dependabot or something else notify us if the latest supported version changes? Otherwise I'm afraid we'll never think to update it. I think I might rather discover that an Ubuntu update breaks our build as soon as it happens rather than months later, even if that means the CI mysteriously failing.

Marcono1234 · 2024-06-01T17:19:54Z

.github/workflows/check-api-compatibility.yml

-          mvn --batch-mode --no-transfer-progress org.codehaus.mojo:versions-maven-plugin:2.11.0:set -DnewVersion=JAPICMP-OLD
-          # Install artifacts with dummy version in local repository; used later by Maven plugin for comparison
-          mvn --batch-mode --no-transfer-progress install -DskipTests
+      - name: Set dummy version


Maybe the "used later by Maven plugin for comparison" should be preserved in some way (e.g. as comment) to make it clearer why this is done.

What about this?

Suggested change

- name: Set dummy version

# First sets a dummy project version, then installs the artifacts with that version in the local

# Maven repository, and in a later step uses it as 'old version' for the API compatibility check.

- name: Set dummy version

Marcono1234 · 2024-06-01T17:30:19Z

Would Dependabot or something else notify us if the latest supported version changes?

No, that does not seem to be supported yet, see dependabot/dependabot-core#7182.

eamonnmcmanus · 2024-06-01T18:40:12Z

Would Dependabot or something else notify us if the latest supported version changes?

No, that does not seem to be supported yet, see dependabot/dependabot-core#7182.

Thanks for that pointer. In that case, I'd suggest we leave this part as it is (with ubuntu-latest) until Dependabot does have that support.

ceddy4395 force-pushed the fix-gha-smells branch from 3cd6a0c to e558c86 Compare May 24, 2024 11:43

Fix gha smells:

f136145

- Use fixed version for runs-on argument - Avoid jobs without timeouts - Steps should only perform a single command

ceddy4395 force-pushed the fix-gha-smells branch from e558c86 to f136145 Compare May 24, 2024 11:46

ceddy4395 changed the title ~~Fix gha smells:~~ Fix potential github action smells May 24, 2024

Marcono1234 reviewed May 24, 2024

View reviewed changes

ceddy4395 added 2 commits May 27, 2024 14:23

Include suggestions from PR:

1bde077

- Increase timeout - Change name for build step - Move documentation closer to run.

Fix gha smells in other workflows as well:

2b20b0b

- Use fixed version for runs-on argument - Avoid jobs without timeouts - Steps should only perform a single command

Marcono1234 reviewed Jun 1, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix potential github action smells #2684

Fix potential github action smells #2684

ceddy4395 commented May 24, 2024 •

edited

Loading

google-cla bot commented May 24, 2024

Marcono1234 left a comment •

edited

Loading

Marcono1234 May 24, 2024

ceddy4395 May 27, 2024

Marcono1234 May 24, 2024

ceddy4395 May 27, 2024

Marcono1234 May 24, 2024

ceddy4395 May 27, 2024

Marcono1234 May 24, 2024 •

edited

Loading

ceddy4395 May 27, 2024

ceddy4395 commented May 27, 2024

eamonnmcmanus commented May 29, 2024

Marcono1234 Jun 1, 2024

Marcono1234 commented Jun 1, 2024

eamonnmcmanus commented Jun 1, 2024

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

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

Fix potential github action smells #2684

Are you sure you want to change the base?

Fix potential github action smells #2684

Conversation

ceddy4395 commented May 24, 2024 • edited Loading

Purpose

Description

Checklist

google-cla bot commented May 24, 2024

Marcono1234 left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Marcono1234 May 24, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ceddy4395 commented May 27, 2024

eamonnmcmanus commented May 29, 2024

Choose a reason for hiding this comment

Marcono1234 commented Jun 1, 2024

eamonnmcmanus commented Jun 1, 2024

ceddy4395 commented May 24, 2024 •

edited

Loading

Marcono1234 left a comment •

edited

Loading

Marcono1234 May 24, 2024 •

edited

Loading