Set cache option on setup-go action in workflows #8280
Conversation
Signed-off-by: Brooke Hamilton <[email protected]>
brooke-hamilton
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
brooke-hamilton
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #8280 +/- ##
==========================================
- Coverage 59.94% 59.93% -0.02%
==========================================
Files 596 596
Lines 40432 40432
==========================================
- Hits 24238 24232 -6
- Misses 14367 14371 +4
- Partials 1827 1829 +2 ☔ View full report in Codecov by Sentry. |
Signed-off-by: Brooke Hamilton <[email protected]>
brooke-hamilton
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
brooke-hamilton
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
brooke-hamilton
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
This comment has been minimized.
This comment has been minimized.
|
⌛ Building Radius and pushing container images for functional tests... |
brooke-hamilton
temporarily deployed
to
functional-tests
GitHub Actions
Inactive
brooke-hamilton
temporarily deployed
to
publish-bicep
GitHub Actions
Inactive
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
There was a problem hiding this comment.
Is this not being used anymore?
There was a problem hiding this comment.
I could not find any references to this action in the radius-project GitHub org.
| @@ -101,6 +101,7 @@ jobs: | |||
| with: | |||
| go-version: ${{ env.GOVER }} | |||
| cache-dependency-path: go.sum | |||
| cache: true | |||
There was a problem hiding this comment.
I think this is true by default and does not need to be set explicitly.
There was a problem hiding this comment.
When this wasn't added, which means that it was true, we were getting the same error.
There was a problem hiding this comment.
I added some information here: #8147. Maybe you can find the missing link.
There was a problem hiding this comment.
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?
- Remove separate caching steps that used
cache@v4and set thesetup-goaction propertycache: true. (this pr) - Update our
go.modto use a version greater than 1.23.0 and remove hard coded go versions from the workflow. (next pr) - If we still see caching errors after doing the above two steps, set
cache: falseon thesetup-goaction.
I really appreciate your feedback on this - I know you have a lot of knowledge in this area having spent significant cycles on it. ❤️
Radius functional test overview
Click here to see the list of tools in the current test run
Test Status⌛ Building Radius and pushing container images for functional tests... |
Description
This PR sets the
cache: trueoption on thesetup-goactions in the workflows. Previously we used a common caching technique to cache the go install and modules folders, but this feature is now built into thesetup-goaction. This change was tested with PR #8276. Following that, this PR updates the remaining workflows to use the new caching feature.The default value for the
cacheoption istrueon thesetup-go@v5action. This PR explicitly sets the value totrueon the action where it is not specified so that the option will be obvious, even though it will not change the behavior of the action for cases when the option was not set.build.yaml: Explicitly set cache option to true, which is the same as the default value.functional-test-cloud.yaml: Changed the cache option from false to true and removed the original caching actions.lint.yaml: Explicitly set cache option to true, which is the same as the default value.long-running-azure.yaml: Explicitly set cache option to true, which is the same as the default value.publish-docs.yaml: Explicitly set cache option to true, which is the same as the default value.Type of change
Related to issue: #8274
Related to PR #8276
Contributor checklist
Please verify that the PR meets the following requirements, where applicable: