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

Optimize CI a bit #574

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

Optimize CI a bit #574

wants to merge 8 commits into from

Conversation

Goooler
Copy link
Contributor

@Goooler Goooler commented Mar 22, 2022

Mainly for using gradle-build-action to speed up build on CI, and bump action versions by the way.

@Goooler
Copy link
Contributor Author

Goooler commented Mar 22, 2022

CC @JLLeitschuh

@Goooler
Copy link
Contributor Author

Goooler commented May 16, 2022

@JLLeitschuh CC

JLLeitschuh added a commit that referenced this pull request Mar 31, 2023
This will unblock #574

Signed-off-by: Jonathan Leitschuh <[email protected]>
JLLeitschuh added a commit that referenced this pull request Mar 31, 2023
This will unblock #574

Signed-off-by: Jonathan Leitschuh <[email protected]>
@Goooler
Copy link
Contributor Author

Goooler commented Apr 4, 2023

You can update to Gradle 8 or 7.6 to support Java 19.

@JLLeitschuh
Copy link
Owner

You can update to Gradle 8 or 7.6 to support Java 19.

That's the plan. But things are breaking, and I don't have the time to figure out what's going on right now: #660

@Goooler
Copy link
Contributor Author

Goooler commented Jan 20, 2024

Validation job now only be executed after wrapper files are changed, it could be removed from required status checks.

@Goooler
Copy link
Contributor Author

Goooler commented Feb 2, 2024

CC @wakingrufus @JLLeitschuh

Copy link
Owner

@JLLeitschuh JLLeitschuh left a comment

Choose a reason for hiding this comment

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

Some things to resolve. Much appreciated for the assistance

restore-keys: |
${{ runner.os }}-gradle-cache-
# Inspired by https://github.com/actions/cache/issues/432#issuecomment-740376179
- name: Restore TestKit cache
Copy link
Owner

Choose a reason for hiding this comment

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

Does this now get handled by the Gradle Action?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nope, if we still need to cache testKit dir, can we store them to ~/.gradle/.gradle-test-kit?

Copy link
Owner

Choose a reason for hiding this comment

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

Can you make sure this action is being run with no credentials?

runs-on: ubuntu-latest

if: github.repository == 'JLLeitschuh/ktlint-gradle'
Copy link
Owner

Choose a reason for hiding this comment

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

Why? What value does this if check add?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No much necessary to run these jobs in forked repos.

@@ -1,12 +1,18 @@
name: "Validate Gradle Wrapper"
on: [push, pull_request]
Copy link
Owner

Choose a reason for hiding this comment

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

I'd prefer to keep these. And I don't know if the filtering adds much value. It actual adds additional risk, as it completely misses the second Gradle wrapper in the plugins directory

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Reverted.

Comment on lines +13 to +15
GRADLE_PUBLISH_KEY: ${{ secrets.GRADLE_PUBLISH_KEY }}
GRADLE_PUBLISH_SECRET: ${{ secrets.GRADLE_PUBLISH_SECRET }}
GITHUB_KEY: ${{ secrets.GithubKey }}
Copy link
Owner

Choose a reason for hiding this comment

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

Why move these to the top when they are only needed for 2 steps. I don't see value in exposing these env variables to other action steps.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just reuse them for steps.

@@ -32,7 +33,7 @@ jobs:

steps:
- name: "Checkout code"
uses: actions/checkout@93ea575cb5d8a053eaa0ac8fa3b40d7e05a33cc8 # v3.1.0
uses: actions/checkout@4
Copy link
Owner

Choose a reason for hiding this comment

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

Using the exact commit hash is done intentionally.

It's a suggested security measure, advised by security scorecard.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We can pin them to hash, but there would be better to enable @renovate-bot for this repo to keep thing up to date.

Copy link
Owner

Choose a reason for hiding this comment

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

Dependabot can assist here as well I believe

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