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

all: align Go support with upstream policy #2705

Merged
merged 5 commits into from
Jun 26, 2024

Conversation

nsrip-dd
Copy link
Contributor

@nsrip-dd nsrip-dd commented May 22, 2024

What does this PR do?

Align our Go support policy with the official policy of the Go project

Motivation

The Go project supports only the two most recent language versions. See
https://go.dev/doc/devel/release#policy. Older language versions do not
receive bug/security fixes. This commit aligns the dd-trace-go support
policy with the official one, dropping support for the third most recent
language version (1.20 as of this commit). This will let us more quickly
adopt new features from Go, such as structured logging from slog or
runtime execution trace flight recording, or any other new APIs added in
the future.

Reviewer's Checklist

  • Changed code has unit tests for its functionality at or near 100% coverage.
  • System-Tests covering this feature have been added and enabled with the va.b.c-dev version tag.
  • There is a benchmark for any new code, or changes to existing code.
  • If this interacts with the agent in a new way, a system test has been added.
  • Add an appropriate team label so this PR gets put in the right place for the release notes.
  • Non-trivial go.mod changes, e.g. adding new modules, are reviewed by @DataDog/dd-trace-go-guild.

Unsure? Have a question? Request a review!

@nsrip-dd nsrip-dd force-pushed the nick.ripley/tighten-go-support-policy branch from 277175e to 613c797 Compare May 22, 2024 15:52
@pr-commenter
Copy link

pr-commenter bot commented May 22, 2024

Benchmarks

Benchmark execution time: 2024-06-26 14:57:39

Comparing candidate commit 7ba62b2 in PR branch nick.ripley/tighten-go-support-policy with baseline commit b70405a in branch main.

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

Copy link
Contributor

This PR is stale because it has been open 20 days with no activity. Remove stale label or comment or this will be closed in 10 days.

@github-actions github-actions bot added the stale Stuck for more than 1 month label Jun 12, 2024
@nsrip-dd nsrip-dd removed the stale Stuck for more than 1 month label Jun 25, 2024
@nsrip-dd nsrip-dd force-pushed the nick.ripley/tighten-go-support-policy branch 4 times, most recently from 12d2e81 to b5d7300 Compare June 26, 2024 14:00
@nsrip-dd nsrip-dd changed the title [WIP] match Go support with upstream policy all: align Go support with upstream policy Jun 26, 2024
The Go project supports only the two most recent language versions. See
https://go.dev/doc/devel/release#policy. Older language versions do not
receive bug/security fixes. This commit aligns the dd-trace-go support
policy with the official one, dropping support for the third most recent
language version (1.20 as of this commit). This will let us more quickly
adopt new features from Go, such as structured logging from slog or
runtime execution trace flight recording, or any other new APIs added in
the future.
@nsrip-dd nsrip-dd force-pushed the nick.ripley/tighten-go-support-policy branch from b5d7300 to 9a418bb Compare June 26, 2024 14:03
@nsrip-dd nsrip-dd marked this pull request as ready for review June 26, 2024 14:03
@nsrip-dd nsrip-dd requested review from a team as code owners June 26, 2024 14:03
Copy link
Member

@darccio darccio left a comment

Choose a reason for hiding this comment

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

LGTM, but I think we should be careful with any toolchain directive in our go.mod files.

go 1.20
go 1.21

toolchain go1.22.2
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
toolchain go1.22.2
toolchain go1.21.11

Copy link
Contributor

@eliottness eliottness left a comment

Choose a reason for hiding this comment

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

Can you remove this if block here ? I also seems mentions to go 1.20 in the CI in multios-unit-tests.yml and in parametric.yml. Can we switch to stable ?

nsrip-dd added 4 commits June 26, 2024 10:22
It was previously at 1.22.2 (what I ran go mod tidy with locally).
However, that would mean that we wouldn't get any 1.21 test coverage as
the 1.21 toolchain would download the 1.22.2 toolchain and run the tests
with that.
We run parametric tests with the oldest Go language version we support.
As we're now aligning with the official Go support policy, that would
now be what the Go GitHub Actions tooling calls "oldstable".
@nsrip-dd
Copy link
Contributor Author

nsrip-dd commented Jun 26, 2024

Can you remove this if block here ? I also seems mentions to go 1.20 in the CI in multios-unit-tests.yml and in parametric.yml. Can we switch to stable ?

@eliottness Done, thanks! I opted for "oldstable" since that seems to match what we did before (use the oldest supported version). Related, I wanted to use "oldstable" and "stable" everywhere but there were a few places where we need to explicitly provide the Go version. I considered some kind of templating so we could define oldstable & stable in one place but I didn't see an easy way to do that with GitHub Actions. Perhaps defining variables for the repository (like secrets, but not secret)? Can do that as a followup.

@nsrip-dd nsrip-dd merged commit 3200ef3 into main Jun 26, 2024
165 checks passed
@nsrip-dd nsrip-dd deleted the nick.ripley/tighten-go-support-policy branch June 26, 2024 15:16
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.

3 participants