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

Introduce coverage to PR pipelines #5357

Merged
merged 18 commits into from
May 4, 2022
Merged
Show file tree
Hide file tree
Changes from 14 commits
Commits
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
26 changes: 21 additions & 5 deletions .drone/drone.jsonnet
Original file line number Diff line number Diff line change
Expand Up @@ -46,15 +46,15 @@ local github_secret = secret('github_token', 'infra/data/ci/github/grafanabot',
// Injected in a secret because this is a public repository and having the config here would leak our environment names
local deploy_configuration = secret('deploy_config', 'infra/data/ci/loki/deploy', 'config.json');


local run(name, commands) = {
local run(name, commands, env={}) = {
name: name,
image: 'grafana/loki-build-image:%s' % build_image_version,
commands: commands,
environment: env,
};

local make(target, container=true) = run(target, [
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target,
local make(target, container=true, args=[]) = run(target, [
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args),
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
local make(target, container=true, args=[]) = run(target, [
'make ' + (if !container then 'BUILD_IN_CONTAINER=false ' else '') + target + ' ' + std.join(' ', args),
local make(target, container=true, args=[]) = run(target, [
std.join(' ', [
'make',
'BUILD_IN_CONTAINER=' + container,
target,
] + args),

This will get rid of the unrelated changes in drone.yml

]);

local docker(arch, app) = {
Expand Down Expand Up @@ -345,7 +345,23 @@ local manifest(apps) = pipeline('manifest') {
steps: [
make('check-drone-drift', container=false) { depends_on: ['clone'] },
make('check-generated-files', container=false) { depends_on: ['clone'] },
make('test', container=false) { depends_on: ['clone', 'check-generated-files'] },
make('test', container=false) { depends_on: ['clone'] },
run('clone-main', commands=['cd ..', 'git clone $CI_REPO_REMOTE loki-main', 'cd -']),
run('test-main', commands=['cd ../loki-main', 'BUILD_IN_CONTAINER=false make test']) { depends_on: ['clone-main'] },
make('compare-coverage', container=false, args=[
'old=../loki-main/test_results.txt',
'new=test_results.txt',
'packages=ingester,distributor,querier,ruler,storage',
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'packages=ingester,distributor,querier,ruler,storage',
'packages=ingester,distributor,querier,iter,storage',

The ruler is not super critical here, but the iter package has far-reaching consequences.
Maybe @cyriltovena can suggest any other critical packages we need to cover if there are any?

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 changed the list to these: ingester,distributor,querier,iter,storage,chunkenc,logql,loki

I think we can be fairly aggressive here because we ought to have color coding to help us see where to focus.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd like to also add querier/queryrange.

Copy link
Contributor

Choose a reason for hiding this comment

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

does it fail if the package name has changed ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We'd see a large diff, but this won't fail

'> diff.txt',
]) { depends_on: ['test', 'test-main'] },
run('report-coverage', commands=[
"pull=$(echo $CI_COMMIT_REF | awk -F '/' '{print $3}')",
"body=$(jq -Rs '{body: . }' diff.txt)",
'curl -X POST -u $USER:$TOKEN -H "Accept: application/vnd.github.v3+json" https://api.github.com/repos/grafana/loki/issues/$pull/comments -d "$body" > /dev/null',
], env={
USER: 'grafanabot',
TOKEN: { from_secret: github_secret.name },
}) { depends_on: ['compare-coverage'] },
make('lint', container=false) { depends_on: ['clone', 'check-generated-files'] },
make('check-mod', container=false) { depends_on: ['clone', 'test', 'lint'] },
{
Expand Down
86 changes: 66 additions & 20 deletions .drone/drone.yml
Original file line number Diff line number Diff line change
Expand Up @@ -36,60 +36,104 @@ kind: pipeline
name: check
steps:
- commands:
- make BUILD_IN_CONTAINER=false check-drone-drift
- 'make BUILD_IN_CONTAINER=false check-drone-drift '
depends_on:
- clone
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: check-drone-drift
- commands:
- make BUILD_IN_CONTAINER=false check-generated-files
- 'make BUILD_IN_CONTAINER=false check-generated-files '
depends_on:
- clone
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: check-generated-files
- commands:
- make BUILD_IN_CONTAINER=false test
- 'make BUILD_IN_CONTAINER=false test '
depends_on:
- clone
- check-generated-files
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: test
- commands:
- make BUILD_IN_CONTAINER=false lint
- cd ..
- git clone $CI_REPO_REMOTE loki-main
- cd -
environment: {}
image: grafana/loki-build-image:0.21.0
name: clone-main
- commands:
- cd ../loki-main
- BUILD_IN_CONTAINER=false make test
depends_on:
- clone-main
environment: {}
image: grafana/loki-build-image:0.21.0
name: test-main
- commands:
- make BUILD_IN_CONTAINER=false compare-coverage old=../loki-main/test_results.txt
new=test_results.txt packages=ingester,distributor,querier,ruler,storage > diff.txt
depends_on:
- test
- test-main
environment: {}
image: grafana/loki-build-image:0.21.0
name: compare-coverage
- commands:
- pull=$(echo $CI_COMMIT_REF | awk -F '/' '{print $3}')
- 'body=$(jq -Rs ''{body: . }'' diff.txt)'
- 'curl -X POST -u $USER:$TOKEN -H "Accept: application/vnd.github.v3+json" https://api.github.com/repos/grafana/loki/issues/$pull/comments
-d "$body" > /dev/null'
depends_on:
- compare-coverage
environment:
TOKEN:
from_secret: github_token
USER: grafanabot
image: grafana/loki-build-image:0.21.0
name: report-coverage
- commands:
- 'make BUILD_IN_CONTAINER=false lint '
depends_on:
- clone
- check-generated-files
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: lint
- commands:
- make BUILD_IN_CONTAINER=false check-mod
- 'make BUILD_IN_CONTAINER=false check-mod '
depends_on:
- clone
- test
- lint
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: check-mod
- commands:
- apk add make bash && make lint-scripts
image: koalaman/shellcheck-alpine:stable
name: shellcheck
- commands:
- make BUILD_IN_CONTAINER=false loki
- 'make BUILD_IN_CONTAINER=false loki '
depends_on:
- clone
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: loki
- commands:
- make BUILD_IN_CONTAINER=false validate-example-configs
- 'make BUILD_IN_CONTAINER=false validate-example-configs '
depends_on:
- loki
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: validate-example-configs
- commands:
- make BUILD_IN_CONTAINER=false check-example-config-doc
- 'make BUILD_IN_CONTAINER=false check-example-config-doc '
depends_on:
- clone
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: check-example-config-doc
trigger:
event:
Expand All @@ -103,9 +147,10 @@ kind: pipeline
name: mixins
steps:
- commands:
- make BUILD_IN_CONTAINER=false lint-jsonnet
- 'make BUILD_IN_CONTAINER=false lint-jsonnet '
depends_on:
- clone
environment: {}
image: grafana/jsonnet-build:c8b75df
name: lint-jsonnet
trigger:
Expand All @@ -123,7 +168,8 @@ node:
steps:
- commands:
- go test -mod=vendor -bench=Benchmark -benchtime 20x -timeout 120m ./pkg/...
image: grafana/loki-build-image:0.20.0
environment: {}
image: grafana/loki-build-image:0.21.0
name: All
trigger:
cron:
Expand Down Expand Up @@ -1064,6 +1110,6 @@ kind: secret
name: deploy_config
---
kind: signature
hmac: ca18b0336abbfa2af076bcf301c13450ce1a8cdad68b0d7c4a5a3e6fbb6a3140
hmac: 8d723b3c02513bbeca89e5f7cbedb4c0c132ad95e6745b943c3e15aa30593218

...
3 changes: 2 additions & 1 deletion .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ dlv
rootfs/
dist
coverage.txt
test_results.txt
.DS_Store
.aws-sam
.idea
Expand All @@ -36,4 +37,4 @@ coverage.txt
# terraform
.terraform*
*.tfstate*
*.tfvars
*.tfvars
9 changes: 6 additions & 3 deletions Makefile
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
.PHONY: validate-example-configs generate-example-config-doc check-example-config-doc
.PHONY: clean clean-protos

SHELL = /usr/bin/env bash
SHELL = /usr/bin/env bash -o pipefail

# Empty value = no -mod parameter is used.
# If not empty, GOMOD is passed to -mod= parameter.
Expand Down Expand Up @@ -44,7 +44,7 @@ DOCKER_IMAGE_DIRS := $(patsubst %/Dockerfile,%,$(DOCKERFILES))
BUILD_IN_CONTAINER ?= true

# ensure you run `make drone` after changing this
BUILD_IMAGE_VERSION := 0.20.0
BUILD_IMAGE_VERSION := 0.21.0

# Docker image info
IMAGE_PREFIX ?= grafana
Expand Down Expand Up @@ -276,7 +276,10 @@ lint:
########

test: all
GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./...
GOGC=10 $(GOTEST) -covermode=atomic -coverprofile=coverage.txt $(MOD_FLAG) -p=4 ./... | tee test_results.txt
Copy link
Contributor

Choose a reason for hiding this comment

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

You need to update the SHELL variable in the Makefile to use bash -o pipefail
Otherwise the exit code from go test is not propagated. I ran into the same issue once :D


compare-coverage:
./tools/diff_coverage.sh $(old) $(new) $(packages)

#########
# Clean #
Expand Down
2 changes: 1 addition & 1 deletion loki-build-image/Dockerfile
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ RUN apt-get update && \
musl gnupg ragel \
file zip unzip jq gettext\
protobuf-compiler libprotobuf-dev \
libsystemd-dev && \
libsystemd-dev jq && \
rm -rf /var/lib/apt/lists/* /tmp/* /var/tmp/*

COPY --from=docker /usr/bin/docker /usr/bin/docker
Expand Down
18 changes: 18 additions & 0 deletions tools/diff_coverage.sh
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
#!/bin/bash

if [[ ! -f "$1" ]] || [[ ! -f "$2" ]]; then
echo "unable to compare test coverage: both old and new files must exist"
exit 0
fi

echo '```diff'
for pkg in ${3//,/ }; do
old=$(grep "pkg/${pkg}\s" "$1" | sed s/%// | awk '{print $5}')
new=$(grep "pkg/${pkg}\s" "$2" | sed s/%// | awk '{print $5}')
echo | awk -v pkg="${pkg}" -v old="${old:-0}" -v new="${new:-0}" \
'{
sign=new - old < 0 ? "-" : "+"
printf ("%s %11s\t%s\n", sign, pkg, new - old)
}'
done
echo '```'