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

feat: only cache if the SDK is not yet cached #187

Merged
merged 1 commit into from
Oct 11, 2023
Merged

Conversation

quad
Copy link

@quad quad commented Oct 5, 2023

No description provided.

@quad quad requested a review from amyu as a code owner October 5, 2023 14:05
Copy link
Owner

@amyu amyu left a comment

Choose a reason for hiding this comment

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

Thanks PR!
This is a great change!

I didn't know about lookupOnly!
Please see I commented Suggestion

src/cache.ts Outdated
Comment on lines 59 to 70
core.info(`checking if "${restoreKey}" is already cached ...`)
const hasEntry = await cache.restoreCache([ANDROID_HOME_DIR], restoreKey, [], { lookupOnly: true })
if (hasEntry) {
core.info(`Found in cache: ${restoreKey}`)
return
}

Copy link
Owner

@amyu amyu Oct 6, 2023

Choose a reason for hiding this comment

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

Suggested change
core.info(`checking if "${restoreKey}" is already cached ...`)
const hasEntry = await cache.restoreCache([ANDROID_HOME_DIR], restoreKey, [], { lookupOnly: true })
if (hasEntry) {
core.info(`Found in cache: ${restoreKey}`)
return
}
core.info(`checking if "${restoreKey}" is already cached ...`)
const hasEntry = await cache.restoreCache([ANDROID_HOME_DIR], restoreKey, [], { lookupOnly: true })
if (hasEntry) {
core.info(`Found in cache: ${restoreKey}`)
return
}
core.info(`caching "${restoreKey}" ...`)
return await cache.saveCache([ANDROID_HOME_DIR], restoreKey)
}

Copy link
Author

Choose a reason for hiding this comment

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

Are you suggesting that I remove the catch on ReserveCacheError? 👌

Copy link
Owner

Choose a reason for hiding this comment

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

Yes!
I did not know how to check if the cache already exists.
So, if I did a cache and got a ReserveCacheError, it meant that the cache was already there.

@amyu
Copy link
Owner

amyu commented Oct 6, 2023

please run npm run all and commit!

@amyu
Copy link
Owner

amyu commented Oct 6, 2023

and check the lint error(prettier)

@amyu
Copy link
Owner

amyu commented Oct 11, 2023

Thanks for your PR!

@amyu amyu merged commit adc2ade into amyu:main Oct 11, 2023
@amyu
Copy link
Owner

amyu commented Oct 11, 2023

@quad
Copy link
Author

quad commented Oct 11, 2023

Thank you!

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