ci: update linter job and add bodyclose #1942

ahmed-mez · 2023-04-26T15:59:01Z

What does this PR do?

Remove deprecated lint job
Make all teams own linter rules
Add bodyclose linter rule
Apply bodyclose fixes

Motivation

Remove deprecated tooling
Safer code by adding the bodyclose rule
All teams own the linter rules to make sure every team approves any future changes in the linter rules

Describe how to test/QA your changes

The CI passes

Reviewer's Checklist

Changed code has unit tests for its functionality.
If this interacts with the agent in a new way, a system test has been added.

pr-commenter · 2023-04-26T16:03:31Z

Benchmarks

Comparing candidate commit 446c2c8 in PR branch ahmed/linter-updates with baseline commit d3d375c in branch main.

Found 0 performance improvements and 0 performance regressions! Performance is the same for 18 metrics, 0 unstable metrics.

ahmed-mez · 2023-04-26T17:37:41Z

contrib/twitchtv/twirp/twirp_test.go

In this contrib wc.Do(req) returns a nil error and a nil response, that's why it's skipped.

ajgajg1134

Looks good! 🎉

rarguelloF

Thanks for the PR!! 🎉
LGTM, left some questions / small suggestion

rarguelloF · 2023-04-27T08:42:12Z

contrib/net/http/roundtripper_test.go

@@ -166,7 +170,8 @@ func TestRoundTripperNetworkError(t *testing.T) {
 		Timeout:   1 * time.Millisecond,
 	}

-	client.Get(s.URL + "/hello/world")
+	_, err := client.Get(s.URL + "/hello/world") //nolint:bodyclose


question: why is it skipped here?

We expect a non-nil error, so the response is always nil.

rarguelloF · 2023-04-27T08:42:27Z

contrib/net/http/roundtripper_test.go

@@ -205,7 +210,8 @@ func TestRoundTripperNetworkErrorWithErrorCheck(t *testing.T) {
 			Timeout:   1 * time.Millisecond,
 		}

-		client.Get(s.URL + "/hello/world")
+		_, err := client.Get(s.URL + "/hello/world") //nolint:bodyclose


(same question here)

same answer

rarguelloF · 2023-04-27T08:44:51Z

internal/httpmem/httpmem_test.go

+	resp, err := c.Do(r)
+	if err != nil {
+		return
 	}
+	defer resp.Body.Close()
+	t.Fatal("request should have failed, but it succeeded")


Suggested change

resp, err := c.Do(r)

if err != nil {

return

}

defer resp.Body.Close()

t.Fatal("request should have failed, but it succeeded")

_, err = c.Do(r)

require.Error(t, err, "request should have failed, but it succeeded")

would the linter force you to close the body in this case?

See https://stackoverflow.com/questions/18598780/is-resp-body-close-necessary-if-we-dont-read-anything-from-the-body

Hellzy

Looking good on the AppSec side, thanks for this @ahmed-mez 👍

…ch as queuename tags contrib: upgrade labstack/echo/v4 from v4.2.0 to v4.9.0 (#1891) ci: fix flaky lint job (#1892) contrib/elasticsearch: use naming schema (#1897) ci: introduce golangci (#1898) appsec: suspicious request blocking (#1797) Co-authored-by: Julio Guerra <julio@datadog.com> ci/golangci-lint: skip google.golang.org/grpc.v12 (#1899) .github/workflows: run ASM and RC system-tests scenarios (#1900) contrib/hashicorp/vault: use naming schema (#1868) contrib/database/sql: add WithIgnoreQueryTypes option (#1823) Co-authored-by: Zarir Hamza <zarir.hamza@datadoghq.com> Co-authored-by: Rodrigo Argüello <rodrigo.arguello@datadoghq.com> contrib/database/sql: use naming schema (#1895) internal/appsec: add server.request.method address (#1893) Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> internal/appsec/dyngo: atomic instrumentation swapping (#1873) Co-authored-by: François Mazeau <francois.mazeau@datadoghq.com> go.mod: datadog-agent/pkg/remoteconfig/state 7.45.0-rc.1 (#1902) internal/version: bump to v1.51.0 (#1912) ddtrace/tracer: don't set empty tracestate propagation tag (#1910) go.mod: github.com/DataDog/datadog-agent/pkg/obfuscate 7.45.0-rc.1 (#1916) appsec: add blocking SDK body operation (#1901) * Modifying the appsec api: adding appsec.MonitorParsedHTTPBody an error as return value * Adding a call to the WAF to check for security event synchronously with a call to appsec.MonitorParsedHTTPBody on the body passed as parameter * Removing the call to the WAF done on the body an the end of a request because we moved it. * Refactoring the waf addresses storage and access Signed-off-by: Eliott Bouhana <eliott.bouhana@datadoghq.com> ddtrace/{opentelemetry,opentracer}: add telemetry (#1909) internal/appsec: fix user ID event detection (#1918) internal/telemetry: track tracer init time metric (#1896) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> internal/appsec/remoteconfig: fix rules overrides (#1921) contrib/mongodb: use naming schema (#1908) contrib/syndtr/goleveldb/leveldb: use naming schema (#1914) contrib/tidwall/buntdb: use naming schema (#1913) internal/appsec: do not ignore the appsec events rate limiter (#1927) remoteconfig: remove empty products and don't override appsec rules data (#1925) contrib/kafka: refactor tests (#1907) contrib/google.golang.org/grpc: use naming schema (#1919) contrib/twitchtv/twirp: use naming schema (#1920) contrib/http: use naming schema (#1929) ddtrace/tracer: reset decision maker during fallback behavior of w3c header extraction (#1933) contrib/cassandra: use naming schema (#1911) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> contrib/redis: use naming schema (#1906) Co-authored-by: Andrew Glaude <andrew.glaude@datadoghq.com> ci/system-tests: more scenarios with parallel jobs (#1938) ci: update linter job and add bodyclose (#1942) contrib/redis/go-redis.v9: support v9 (#1730) Add support for new go-redis version v9. It does 2 things: Copy existing version 8 files to a new path, /redis/go-redis.v9. Make changes to support version 9. Fixes #1710 format and rerun go tidy get rid of prints add topLevelRegion assertions remove confusing named return values and todo comment ddtrace/tracer: ensure access to trace tags is concurrency-safe (#1948) Spancontext marshaling was accessing tracer internal structures without a lock, resulting in a data race and panic. This commit adds a few methods to trace to allow safe access to the tags and propagatingTags members of trace to the marshaling code. Fixes #1944 ddtrace/tracer: mark context updated when SetUser is called (#1949) Fixes a minor logic mistake when setting a user on a span lint and add default switch case refactor resourceNameKey and value assignments restructure functions to be left aligned use internal logger, be less verbose with function names go back to normal switch type and format Set keyTraceID128 on first span in the chunk only (#1946) go.mod: upgrade go-libddwaf to v1.2.0 (#1953) Co-authored-by: Julio Guerra <julio@datadog.com> contrib/database/sql: fix bug where options were always overwritten by register options (#1904) Co-authored-by: Diana Shevchenko <40775148+dianashevchenko@users.noreply.github.com> ci/smoke-tests: update the go.sum file after go get -u (#1957) contrib/net/http: don't set empty string values as span tags (#1956) Do not set span fields when they are not configured so the tracer can put the defaults in. use normal string then derefence rever go.mod and go.sum changes contrib/internal/httptrace: remove naming schema from init (#1960) contrib/graphql: use naming schema (#1926) internal/telemetry: trim the dependencies version prefix v (#1963) contrib/aws: use naming schema (#1931) contrib/cloud.google.com/go/pubsub.v1: use naming schema (#1937) go mod tidy lint and fix test

ahmed-mez added 4 commits April 26, 2023 17:54

remove deprecated lint job

844e860

all teams own linter rules

4857164

add bodyclose rule

ab275f0

bodyclose fixes

446c2c8

ahmed-mez added the ci label Apr 26, 2023

ahmed-mez marked this pull request as ready for review April 26, 2023 16:12

ahmed-mez requested a review from a team as a code owner April 26, 2023 16:12

ahmed-mez requested a review from a team April 26, 2023 16:12

ahmed-mez requested a review from a team as a code owner April 26, 2023 16:12

ahmed-mez commented Apr 26, 2023

View reviewed changes

ajgajg1134 approved these changes Apr 26, 2023

View reviewed changes

rarguelloF reviewed Apr 27, 2023

View reviewed changes

Hellzy approved these changes Apr 27, 2023

View reviewed changes

ahmed-mez merged commit 158fa4c into main Apr 27, 2023

ahmed-mez deleted the ahmed/linter-updates branch April 27, 2023 10:18

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ci: update linter job and add bodyclose #1942

ci: update linter job and add bodyclose #1942

ahmed-mez commented Apr 26, 2023

pr-commenter bot commented Apr 26, 2023

ahmed-mez Apr 26, 2023

ajgajg1134 left a comment

rarguelloF left a comment

rarguelloF Apr 27, 2023

ahmed-mez Apr 27, 2023

rarguelloF Apr 27, 2023

ahmed-mez Apr 27, 2023

rarguelloF Apr 27, 2023

ahmed-mez Apr 27, 2023

ahmed-mez Apr 27, 2023

Hellzy left a comment

ci: update linter job and add bodyclose #1942

ci: update linter job and add bodyclose #1942

Conversation

ahmed-mez commented Apr 26, 2023

What does this PR do?

Motivation

Describe how to test/QA your changes

Reviewer's Checklist

pr-commenter bot commented Apr 26, 2023

Benchmarks

Choose a reason for hiding this comment

ajgajg1134 left a comment

Choose a reason for hiding this comment

rarguelloF left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Hellzy left a comment

Choose a reason for hiding this comment