Skip to content

Conversation

@nineinchnick
Copy link
Member

Description

Cache should not be restored in the pt job, since it doesn't build anything with Maven, and it makes it unnecessary longer by 2–3 minutes. The issue is caused by a boolean input actually being a string: actions/runner#1483

Additional context and related issues

Release notes

(x) This is not user-visible or docs only and no release notes are required.
( ) Release notes are required, please propose a release note for me.
( ) Release notes are required, with the following suggested text:

@cla-bot cla-bot bot added the cla-signed label Feb 13, 2023
@nineinchnick
Copy link
Member Author

@MiguelWeezardo PTAL again, I added a second commit to avoid downloading all dependencies in the pt job. Properly disabling cache there actually made things worse :D

@MiguelWeezardo
Copy link
Member

I think we now have 2 flags where we should have 1.

Is there ever a case where we want to download dependencies while avoiding the cache, or use the cache without pulling missing dependencies?

We should treat caching as a hidden optimization, the action user should just choose if he wants to have dependencies or not when the action finishes.

@nineinchnick
Copy link
Member Author

I think we now have 2 flags where we should have 1.

Is there ever a case where we want to download dependencies while avoiding the cache, or use the cache without pulling missing dependencies?

We should treat caching as a hidden optimization, the action user should just choose if he wants to have dependencies or not when the action finishes.

I agree, but it's not clear from your comment which flag should remain.

@MiguelWeezardo
Copy link
Member

I think we now have 2 flags where we should have 1.
Is there ever a case where we want to download dependencies while avoiding the cache, or use the cache without pulling missing dependencies?
We should treat caching as a hidden optimization, the action user should just choose if he wants to have dependencies or not when the action finishes.

I agree, but it's not clear from your comment which flag should remain.

I think download_dependencies better explains what changes on the runner.

@nineinchnick
Copy link
Member Author

@findepi @hashhar PTAL

@nineinchnick
Copy link
Member Author

@ebyhr can you take a look too?

@nineinchnick
Copy link
Member Author

@kokosing or maybe you can help with this one? It'll shave off 2–3 minutes from every product test job, so it's worth getting in sooner than later.

@kokosing kokosing merged commit 278c19c into trinodb:master Feb 15, 2023
@kokosing
Copy link
Member

Merged, thanks!

@nineinchnick nineinchnick deleted the setup-disable-cache branch February 15, 2023 15:59
@github-actions github-actions bot added this to the 407 milestone Feb 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Development

Successfully merging this pull request may close these issues.

3 participants