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

Improve building process with cache goes dependencies #689

Merged
merged 15 commits into from
Oct 29, 2021
Merged

Improve building process with cache goes dependencies #689

merged 15 commits into from
Oct 29, 2021

Conversation

khanhntd
Copy link
Contributor

@khanhntd khanhntd commented Oct 28, 2021

Description: <Describe what has changed.

Testing: Did three tests with the caches and make test turns from 4m to 25s and make build turns from 20m to roughly 2

  • Image below is one of the testing results:
    image
  • Image below is the original time:
    image

Documentation:

Contribution: Thanks Seth and Jeffrey for helping me with all the documents and samples codes:

cmd/awscollector/Dockerfile Outdated Show resolved Hide resolved
#Samples codes for different OS: https://github.com/actions/cache/blob/main/examples.md#go---modules
#Since we are using Linux, the go packages are in /go/pkg/mod

- name: Cache go dependencies in go.mod
Copy link
Contributor

Choose a reason for hiding this comment

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

These aren't only dependencies but build cache too so don't need to mention the word dependencies

Copy link
Member

Choose a reason for hiding this comment

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

This will cache tests. Do we want to do that? I would say yes since tests related to changed files will rerun if files are changed.

Copy link
Member

Choose a reason for hiding this comment

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

This will only be caching unit tests I think. Not the integration tests.

Copy link
Member

Choose a reason for hiding this comment

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

You might want to create conditionals to cache the build only on pr not on merge

Copy link
Member

@jefchien jefchien Oct 28, 2021

Choose a reason for hiding this comment

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

This will only be caching unit tests I think. Not the integration tests.

Yep, the integration tests aren't run using go test, so the caching wouldn't affect them. Will this cache tests though? I thought based on the documentation, the options we use with make test will prevent caching. https://pkg.go.dev/cmd/go/internal/test

The rule for a match in the cache is that the run involves the same
test binary and the flags on the command line come entirely from a
restricted set of 'cacheable' test flags, defined as -benchtime, -cpu,
-list, -parallel, -run, -short, and -v. If a run of go test has any test
or non-test flags outside this set, the result is not cached. To
disable test caching, use any test flag or argument other than the
cacheable flags.

GOTEST_OPT?= -short -coverprofile coverage.txt -v -race -timeout 180s

Copy link
Member

Choose a reason for hiding this comment

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

Yes you are right

# instead of adding and update requirements like go get does
# Contribute to: https://stackoverflow.com/questions/66356034/what-is-the-difference-between-go-get-command-and-go-mod-download-command
#We shouldn't use go mod tidy since it has the same goals with go get by updating all dependencies and remove all unnecessary requirements
- name: Install go dependencies if cache dependencies did not exist
Copy link
Contributor

Choose a reason for hiding this comment

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

We don't need this step, the build will automatically download needed dependencies and they'll be cached anyways. go mod download tends to download more than actually needed for the build.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting thought !!!. Will change this in the upcoming commit

Comment on lines 117 to 118
restore-keys: |
${{ runner.os }}-go-
Copy link
Member

Choose a reason for hiding this comment

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

This doesn't do anything, does it? Since nothing will ever store in the cache with this key there will never be anything to restore.

Suggested change
restore-keys: |
${{ runner.os }}-go-

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes it does. The restore-keys are the partial key, not the full key itself. What the flow actually is:

  1. When there is an event that has the exact match, the actions restore the file in the cache file
  2. If there are no exact matches, the action searches for partial matches of the restore keys. When the action finds a partial match, the most recent cache is restored to the path directory.
    Here is the document that states my respond:
    https://docs.github.com/en/actions/advanced-guides/caching-dependencies-to-speed-up-workflows#example-using-the-cache-action

Copy link
Contributor Author

@khanhntd khanhntd Oct 28, 2021

Choose a reason for hiding this comment

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

After thinking about cache behaviors, IMO it should be removed since we don't want to cache the old modules.

@codecov-commenter
Copy link

codecov-commenter commented Oct 28, 2021

Codecov Report

Merging #689 (a434923) into main (9153ede) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main     #689   +/-   ##
=======================================
  Coverage   54.54%   54.54%           
=======================================
  Files          11       11           
  Lines         297      297           
=======================================
  Hits          162      162           
  Misses        118      118           
  Partials       17       17           

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 9153ede...a434923. Read the comment docs.

@@ -19,6 +19,7 @@ on:
- main
- release-branch-v*
- dev
- fix-issue-688
Copy link
Member

Choose a reason for hiding this comment

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

Reminder to remove after testing.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes removing it for the final commit.

@jefchien jefchien merged commit 2376080 into aws-observability:main Oct 29, 2021
@khanhntd khanhntd deleted the fix-issue-688 branch October 29, 2021 15:18
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.

6 participants