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

fix: setup-go's cache-control is OptOut #343

Merged
merged 3 commits into from
Dec 21, 2024

Conversation

jmelahman
Copy link
Contributor

setup-go's caching is on by default and opt-out with cache=false, https://github.com/actions/setup-go?tab=readme-ov-file#v4

Fixes this false positive,
screenshot-2024-12-21-09-34-42

@woodruffw
Copy link
Owner

Thanks @jmelahman! I'll review this later today.

@ubiratansoares
Copy link
Contributor

ubiratansoares commented Dec 21, 2024

My bad here, @jmelahman, thank you so much for catching this one!

Too easy to get lost on those booleans when modelling the behaviour of those cache-aware Actions, I guess 🙈

If I got everything correctly, the issue is setup-go actually defines opt-in via the cache input; however the control value is actually a boolean, not a string-as-enum in as we have in setup-node, setup-java and others.

I gave a try on your Workflow locally, setting the control value to ControlValue::Boolean and it fixes the issue

CleanShot 2024-12-21 at 17 44 18@2x

I think the way to go here is moving the control value to ControlValue:Boolean, since the description of the cache input in setup-go makes it clear that true == opt-in for caching

[Edit] for the sake of a comparison, the lookup-only input* in actions/cache/restore defines the opposite semantics, i.e, true == opt-out of cache restoration.

Does that make sense?

@woodruffw
Copy link
Owner

Unless I'm misunderstanding, I think setup-go is indeed opt-out:

The action will try to enable caching unless the cache input is explicitly set to false.

(From the README)

However, I agree that this should be ControlValue::Boolean.

@woodruffw
Copy link
Owner

Oh, this was my misunderstanding -- I forgot about the distinction between CacheControl and caching_by_default. @ubiratansoares's explanation makes sense to me!

Signed-off-by: William Woodruff <william@yossarian.net>
@woodruffw woodruffw added the bugfix Fixes a known bug label Dec 21, 2024
@woodruffw
Copy link
Owner

Thanks for raising this @jmelahman!

@woodruffw woodruffw merged commit db594e2 into woodruffw:main Dec 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Fixes a known bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants