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.

Sign up for GitHub

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

Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
09434d0
first commit to merge and request
khanhntd Oct 26, 2021
c9e9853
add cache dependencies for ubuntu linux based on this example: https:…
khanhntd Oct 27, 2021
641285b
download all dependencies from go.mod and cache
khanhntd Oct 27, 2021
2db63cf
fix ci yaml file format
khanhntd Oct 27, 2021
5e738d8
add comment for cache dependencies
khanhntd Oct 28, 2021
e2c24a5
testing commit
khanhntd Oct 28, 2021
3988504
final test before doing a pr
khanhntd Oct 28, 2021
53a910a
delete branch fix-issue-688 from CI when pushing
khanhntd Oct 28, 2021
7054e19
delete go mod download when not having mod packages, add go build cac…
khanhntd Oct 28, 2021
39c08c3
testing CI once more to know the cache affects on new commits
khanhntd Oct 28, 2021
7872523
avoid merge branch when cache
khanhntd Oct 28, 2021
a64aea2
testing final time before sending a pull request
khanhntd Oct 28, 2021
edb6da0
combine two cache into one for go build and go dependencies
khanhntd Oct 28, 2021
785c85c
delete CI for fix-issue-688 branch
khanhntd Oct 28, 2021
a434923
delete restore key
khanhntd Oct 28, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 24 additions & 0 deletions .github/workflows/CI.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,6 +100,30 @@ jobs:
cat go.mod
ls pkg

#Cache go dependencies before making unit testing and build
#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 •
edited
Loading

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.

aws-otel-collector/Makefile

Line 28 in 9153ede

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

id: cached_go_dependencies
uses: actions/cache@v2
env:
cache-name: cached_go_dependencies
with:
path: |
~/.cache/go-build
~/go/pkg/mod
key: ${{ runner.os }}-go-${{ hashFiles('**/go.sum') }}
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 •
edited
Loading

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.


# Install all dependencies from go.mod and go.sum instead of source code and keep consistent to the requirements
# 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

if: steps.cached_go_dependencies.outputs.cache-hit != 'true'
run: go mod download
- name: Cache binaries
id: cached_binaries
uses: actions/cache@v2
Expand Down
1 change: 0 additions & 1 deletion README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,6 @@
![CD](https://github.com/aws-observability/aws-otel-collector/workflows/CD/badge.svg)
![GitHub release (latest by date)](https://img.shields.io/github/v/release/aws-observability/aws-otel-collector)


### Overview

AWS Distro for OpenTelemetry Collector(AWS OTel Collector) is a AWS supported version of the upstream OpenTelemetry Collector and is distributed by Amazon. It supports the selected components from the OpenTelemetry community. It is fully compatible with AWS computing platforms including EC2, ECS and EKS. It enables users to send telemetry data to AWS CloudWatch Metrics, Traces and Logs backends as well as the other supported backends.
Expand Down
2 changes: 1 addition & 1 deletion cmd/awscollector/Dockerfile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ RUN apk add --update bash git make build-base
# config go
COPY --from=golang:1.17-alpine /usr/local/go/ /usr/local/go/
ENV PATH="/usr/local/go/bin:${PATH}"

RUN go env -w GOPROXY=direct
khanhntd marked this conversation as resolved.
Show resolved Hide resolved
# download go modules ahead to speed up the building
WORKDIR /workspace
COPY go.mod /workspace/go.mod
Expand Down