Skip to content

Delete runner cache and test cache if success#347

Closed
khanhntd wants to merge 4 commits intoaws:masterfrom
khanhntd:check_runner_cache
Closed

Delete runner cache and test cache if success#347
khanhntd wants to merge 4 commits intoaws:masterfrom
khanhntd:check_runner_cache

Conversation

@khanhntd
Copy link
Copy Markdown
Contributor

@khanhntd khanhntd commented Jan 29, 2022

Description of the issue

Currently, there are two issues with time we got with:

  • Should we build again even though the first retry has success?
  • The time for build is exactly the same without cache and with cache

Description of changes

By changing these two,

  • Adding cache to avoid building again for the second retry would reduce time and increase more resources spending on the remaining jobs
  • Delete this path runner/Library/Caches/go-build when cache to reduce the time (testing it out with the PR)

License

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

Tests

Test with a PR and found unexpected results without caching this path runner/Library/Caches/go-build:
https://github.com/aws/amazon-cloudwatch-agent/runs/4990690788?check_suite_focus=true
https://github.com/aws/amazon-cloudwatch-agent/runs/4990745483?check_suite_focus=true

Compared when cached this path runner/Library/Caches/go-build:
https://github.com/aws/amazon-cloudwatch-agent/runs/4990443776?check_suite_focus=true

@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Jan 29, 2022

Codecov Report

Merging #347 (67101a8) into master (0aad31d) will increase coverage by 0.03%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #347      +/-   ##
==========================================
+ Coverage   57.28%   57.32%   +0.03%     
==========================================
  Files         358      359       +1     
  Lines       16405    16474      +69     
==========================================
+ Hits         9398     9444      +46     
- Misses       6470     6489      +19     
- Partials      537      541       +4     
Impacted Files Coverage Δ
translator/translate/metrics/metrics.go 55.17% <0.00%> (-7.58%) ⬇️
translator/defaultKeyCase.go 0.00% <0.00%> (ø)
translator/totomlconfig/toTomlConfig.go 80.00% <0.00%> (ø)
...lator/translate/metrics/drop_origin/drop_origin.go 78.57% <0.00%> (ø)
plugins/outputs/cloudwatch/cloudwatch.go 73.73% <0.00%> (+0.87%) ⬆️
plugins/inputs/demo/demo.go 57.14% <0.00%> (+7.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0aad31d...67101a8. Read the comment docs.

@khanhntd khanhntd mentioned this pull request Jan 31, 2022
@SaxyPandaBear
Copy link
Copy Markdown
Contributor

Did you confirm that removing the macos cache path did not impact the build and test time?

@SaxyPandaBear
Copy link
Copy Markdown
Contributor

BTW if you are running verifications on changes to the GitHub workflow, I think you could change the PR to a Draft state so that it can't be approved and merged. From when I did that for checking flaky tests in CI, you can still run GitHub workflows on a draft PR.

@khanhntd
Copy link
Copy Markdown
Contributor Author

Did you confirm that removing the macos cache path did not impact the build and test time?

Yes I have tested it and showed it in the description. For your convenience, here are the tests

@khanhntd
Copy link
Copy Markdown
Contributor Author

BTW if you are running verifications on changes to the GitHub workflow, I think you could change the PR to a Draft state so that it can't be approved and merged. From when I did that for checking flaky tests in CI, you can still run GitHub workflows on a draft PR.

Nice tips though. Forgot that we could release a draft PR instead of filing one.

@khanhntd khanhntd changed the title delete runner cache and test cache if success Delete runner cache and test cache if success Jan 31, 2022
@jhnlsn
Copy link
Copy Markdown
Contributor

jhnlsn commented Jan 31, 2022

Overall, I don't think you can skip building based on a cache hit. the only thing we are caching is the output of the testing and the dependencies. If code changes, it doesn't look like a new build/test will be triggered.

@khanhntd
Copy link
Copy Markdown
Contributor Author

khanhntd commented Jan 31, 2022

Overall, I don't think you can skip building based on a cache hit. the only thing we are caching is the output of the testing and the dependencies. If code changes, it doesn't look like a new build/test will be triggered.

It only works when retrying the workflow instead of making a new commits. That's why we use github.run_id that would be unique between commits but will be the same when retrying the workflow. Therefore, if the code chanes, the build/test will still be triggered

@jhnlsn
Copy link
Copy Markdown
Contributor

jhnlsn commented Feb 1, 2022

Ok, i see that now, which brings up another issue. This means that you're only caching for the run, removing the original value of cache, which was to cache dependent packages and successful test runs so they didn't have to be downloaded again and the test didn't have to run again of no changes were made.

@jhnlsn
Copy link
Copy Markdown
Contributor

jhnlsn commented Feb 2, 2022

Closing this out out in favor of breaking out each unit test workflow into its own flow

@jhnlsn jhnlsn closed this Feb 2, 2022
sethAmazon added a commit to sethAmazon/amazon-cloudwatch-agent that referenced this pull request Aug 21, 2023
movence pushed a commit that referenced this pull request Nov 29, 2023
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.

4 participants