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

Set cache option on setup-go action in workflows #8280

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
+9 −54
Open

Set cache option on setup-go action in workflows #8280

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
127b30b
set golang cache option
brooke-hamilton Jan 23, 2025
8c2a076
cache setting on setup-go action
brooke-hamilton Jan 23, 2025
6496f73
Merge branch 'main' into brooke-hamilton/set-go-cache
brooke-hamilton Jan 24, 2025
106e56c
Merge branch 'main' into brooke-hamilton/set-go-cache
brooke-hamilton Jan 30, 2025
a2fdb09
Merge branch 'main' into brooke-hamilton/set-go-cache
brooke-hamilton Feb 1, 2025
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
38 changes: 0 additions & 38 deletions .github/actions/setup-rad-cli/action.yaml
View file
Open in desktop
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this not being used anymore?

Copy link
Member Author

Choose a reason for hiding this comment

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

I could not find any references to this action in the radius-project GitHub org.

This file was deleted.

3 changes: 3 additions & 0 deletions .github/workflows/build.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,6 +101,7 @@ jobs:
with:
go-version: ${{ env.GOVER }}
cache-dependency-path: go.sum
cache: true
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this is true by default and does not need to be set explicitly.

Copy link
Contributor

Choose a reason for hiding this comment

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

When this wasn't added, which means that it was true, we were getting the same error.

Copy link
Contributor

Choose a reason for hiding this comment

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

I added some information here: #8147. Maybe you can find the missing link.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this is true by default and does not need to be set explicitly.

Yes you are right: setting cache: true does not change the behavior because it is true by default. I put it here to be explicit because in some of our other workflows we had additional caching logic that was created before the caching feature was available on the setup-go action. I removed the additional caching logic from functional-test-cloud.yaml, and from the non-cloud tests in a previous PR #8280. Setting this value makes it clear that we are doing caching, in case someone looks at the workflow and thinks "why aren't we doing caching like we had before?" But if you prefer to not have this value set explicitly I will remove it.

When this wasn't added, which means that it was true, we were getting the same error.

This workflow wasn't having an error, but the functional-test-cloud.yaml workflow was having occasional errors with cache conflicts, so I removed the cache@v4 action, which I think is now unnecessary due to caching being built into the setup-go action.

I added some information here: #8147. Maybe you can find the missing link.

I think that PR made sense where we did caching with the cache@v4 action, but I still saw cache file conflicts with that step, so with this PR we can try using the built-in caching with the setup-go action. If we don't see good results we can revert, but so far with PR #8280 things seem OK.

However, one caveat on what I said above, after PR #8280 we are still seeing caching errors with the setup-go action. Here is an example. I found two related issues here and here. They recommend updating our go version to > 1.23.0. We need to standardize our go version on the version set in go.mod anyway (because each workflow has a hard coded version that is separate from go.mod). So let's discuss which version and do that in a separate PR.

What do you think about this plan?

  1. Remove separate caching steps that used cache@v4 and set the setup-go action property cache: true. (this pr)
  2. Update our go.mod to use a version greater than 1.23.0 and remove hard coded go versions from the workflow. (next pr)
  3. If we still see caching errors after doing the above two steps, set cache: false on the setup-go action.

I really appreciate your feedback on this - I know you have a lot of knowledge in this area having spent significant cycles on it. ❤️

- name: Parse release version and set environment variables
run: python ./.github/scripts/get_release_version.py
- name: Make build
Expand Down Expand Up @@ -179,6 +180,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true
- name: Login to GitHub Container Registry
uses: docker/login-action@v3
with:
Expand Down Expand Up @@ -275,6 +277,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true
- name: Setup Node.js
uses: actions/setup-node@v4
with:
Expand Down
18 changes: 2 additions & 16 deletions .github/workflows/functional-test-cloud.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -215,6 +215,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true

- name: Generate ID for release
id: gen-id
Expand Down Expand Up @@ -493,22 +494,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: false

- name: Get Go Cache path
id: go-cache-paths
run: |
echo "go-build=$(go env GOCACHE)" >> $GITHUB_OUTPUT
echo "go-mod=$(go env GOMODCACHE)" >> $GITHUB_OUTPUT

- uses: actions/cache@v4
with:
path: |
${{ steps.go-cache-paths.outputs.go-build }}
${{ steps.go-cache-paths.outputs.go-mod }}
key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
restore-keys: |
${{ runner.os }}-go-
cache: true

- name: Download rad CLI
uses: actions/download-artifact@v4
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/lint.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true
- name: Setup NodeJS
uses: actions/setup-node@v4
with:
Expand Down
2 changes: 2 additions & 0 deletions .github/workflows/long-running-azure.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -177,6 +177,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true
- name: Log the summary of build info for new version.
if: steps.skip-build.outputs.SKIP_BUILD != 'true'
continue-on-error: true
Expand Down Expand Up @@ -330,6 +331,7 @@ jobs:
uses: actions/setup-go@v5
with:
go-version: ${{ env.GOVER }}
cache: true
- name: Download rad CLI
if: env.SKIP_BUILD != 'true'
uses: actions/download-artifact@v4
Expand Down
1 change: 1 addition & 0 deletions .github/workflows/publish-docs.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ jobs:
with:
go-version: ${{ env.GOVER }}
cache-dependency-path: radius/go.sum
cache: true
- name: Setup NodeJS
uses: actions/setup-node@v4
with:
Expand Down
Loading