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

Jump to bottom

Make caching more verbose #650

Merged
merged 1 commit into from Jan 19, 2021
Merged

Make caching more verbose #650

merged 1 commit into from Jan 19, 2021

Conversation

rosik
Copy link
Contributor

@rosik rosik commented Nov 25, 2020 •
edited

  • Print cache size when saving cache (similar to restoring)
  • Print restore success (similar to saving)
  • Print cached file list (if debug logging is enabled)

This makes logs of saving and restoring cache more similar to each other.

image

image

Part of actions/cache#471

@rosik
Copy link
Contributor Author

rosik commented Dec 1, 2020

Hi, guys. Could someone review my patch, please?

@rosik
Copy link
Contributor Author

rosik commented Dec 3, 2020

@dhadka @joshmgross @konradpabjan ?

joshmgross
joshmgross reviewed Dec 3, 2020
View reviewed changes
packages/cache/src/cache.ts Outdated
@@ -100,6 +100,7 @@ export async function restoreCache(
options
)

await listTar(archivePath, compressionMethod)
Copy link
Member

Choose a reason for hiding this comment

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

Listing out the contents of the tar file might not be ideal in all scenarios. If users are concerned more about cache speed than log verbosity, we should consider making this optional.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review. I've added it under if (core.isDebug()) conditinal.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'd like to note that @actions/glob also has its own debug output, but it's not the same as listTar:

image

@rosik rosik requested a review from joshmgross December 3, 2020 22:08
@joshmgross joshmgross requested a review from a team December 4, 2020 22:54
@rosik
Copy link
Contributor Author

rosik commented Dec 9, 2020

Ping

@rosik
Copy link
Contributor Author

rosik commented Dec 17, 2020

@joshmgross I've updated the PR a week ago. Do you expect from me any other actions or do I miss something?

@joshmgross
Copy link
Member

@aiqiaoy or @yacaovsnc could you take a look at this PR?

@yacaovsnc
Copy link
Contributor

Thanks @rosik for the PR. Could you please add a test for the listTar method you created? The tests are here: https://github.com/actions/toolkit/blob/main/packages/cache/__tests__/tar.test.ts

@rosik rosik force-pushed the main branch 3 times, most recently from 856f207 to 967b069 Compare December 20, 2020 20:31
@rosik
Copy link
Contributor Author

rosik commented Dec 20, 2020

I've added a unit test, but I'm worried it's not covered by an end-to-end test. The listTar function is only called when ACTIONS_STEP_DEBUG is set, so it's not run in PR checks: https://github.com/actions/toolkit/pull/650/checks?check_run_id=1585714569. What do you think? If it's desirable, could you give me a hint on how to implement it?

@yacaovsnc
Copy link
Contributor

Sorry for the silence, I was out for the last couple weeks. Personally I think it's fine that this isn't covered in the end-to-end test - to test that we need to set ACTIONS_STEP_DEBUG as a secret value.

FWIW, I forked the repo and set ACTIONS_STEP_DEBUG in my repo setting, and saw the additional debug messages. If really desired, an admin of this repo can enable debug? I don't think it blocks this PR anyway.

yacaovsnc
yacaovsnc approved these changes Jan 4, 2021
View reviewed changes
Copy link
Contributor

@yacaovsnc yacaovsnc left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. Looks good to me.

@rosik
Make caching more verbose
bfdba95 
- Print cache size when saving cache similarly to restoring
- Print restore success similarly to saving
- Print cached file list if debug logging is enabled

See also: actions/cache#471
@rosik
Copy link
Contributor Author

rosik commented Jan 7, 2021

If you ask for my opinion, I vote for enabling ACTIONS_STEP_DEBUG in the main repo. It sounds helpful for the coverage.

So, what's next? I've rebased my patch on top of the fresh main branch. Do you expect any other actions from me now?

@rosik
Copy link
Contributor Author

rosik commented Jan 13, 2021

Ping

@rosik
Copy link
Contributor Author

rosik commented Jan 19, 2021

@dhadka @joshmgross @konradpabjan @aiqiaoy @yacaovsnc ping

@yacaovsnc yacaovsnc merged commit 8252049 into actions:main Jan 19, 2021
@yacaovsnc
Copy link
Contributor

@rosik the cache action can include this change after we release a new npm package. Are you waiting for this change?

@konradpabjan let's include this change next time we release. Do you handle npm releases for cache toolkit too?

@rosik
Copy link
Contributor Author

rosik commented Jan 19, 2021

Are you waiting for this change?

So so. We can always invent a workaround for debugging, but it's usually a little bit nasty.

This was referenced Jan 19, 2021
Merged
Closed
@konradpabjan konradpabjan mentioned this pull request Feb 3, 2021
Merged
This was referenced Mar 6, 2021
Merged
Closed
This was referenced Mar 17, 2021
Merged
Closed
Closed
Merged
Merged
Merged
Closed
Closed
Merged
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Merged
Closed
Closed
Closed
Merged
Closed
Closed
Merged
Closed
Closed
Merged
Closed
Closed
@dhadka dhadka mentioned this pull request Apr 12, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@yacaovsnc yacaovsnc yacaovsnc approved these changes

@dhadka dhadka Awaiting requested review from dhadka

@joshmgross joshmgross Awaiting requested review from joshmgross

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@rosik @joshmgross @yacaovsnc