dd-trace-js icon indicating copy to clipboard operation
dd-trace-js copied to clipboard

change release proposal script exclusion label

Open rochdev opened this issue 3 months ago • 14 comments

What does this PR do?

Change release proposal script exclusion label.

Motivation

Support for the dont-land labels was removed a while ago in favour of semver-major. This turns out to be a problem because it impacts our ability to differentiate major changes by forcing us to use semver-minor for major changes. Instead we can switch back to dont-land labels when something needs to be excluded (pretty much only the version bump) and we can include semver major changes in minors because they are hidden behind a version conditional.

rochdev avatar Sep 19 '25 00:09 rochdev

Overall package size

Self size: 12.4 MB Deduped: 112.49 MB No deduping: 112.89 MB

Dependency sizes | name | version | self size | total size | |------|---------|-----------|------------| | @datadog/libdatadog | 0.7.0 | 35.02 MB | 35.02 MB | | @datadog/native-appsec | 10.2.1 | 20.64 MB | 20.65 MB | | @datadog/native-iast-taint-tracking | 4.0.0 | 11.72 MB | 11.73 MB | | @datadog/pprof | 5.10.0 | 9.91 MB | 10.3 MB | | @opentelemetry/core | 1.30.1 | 908.66 kB | 7.16 MB | | protobufjs | 7.5.4 | 2.95 MB | 5.6 MB | | @datadog/wasm-js-rewriter | 4.0.1 | 2.85 MB | 3.58 MB | | @datadog/native-metrics | 3.1.1 | 1.02 MB | 1.43 MB | | @opentelemetry/api | 1.9.0 | 1.22 MB | 1.22 MB | | jsonpath-plus | 10.3.0 | 617.18 kB | 1.08 MB | | import-in-the-middle | 1.14.2 | 122.36 kB | 850.93 kB | | lru-cache | 10.4.3 | 804.3 kB | 804.3 kB | | opentracing | 0.14.7 | 194.81 kB | 194.81 kB | | source-map | 0.7.6 | 185.63 kB | 185.63 kB | | pprof-format | 2.2.1 | 163.06 kB | 163.06 kB | | @datadog/sketches-js | 2.1.1 | 109.9 kB | 109.9 kB | | lodash.sortby | 4.7.0 | 75.76 kB | 75.76 kB | | ignore | 7.0.5 | 63.38 kB | 63.38 kB | | istanbul-lib-coverage | 3.2.2 | 34.37 kB | 34.37 kB | | rfdc | 1.4.1 | 27.15 kB | 27.15 kB | | dc-polyfill | 0.1.10 | 26.73 kB | 26.73 kB | | @isaacs/ttlcache | 1.4.1 | 25.2 kB | 25.2 kB | | tlhunter-sorted-set | 0.1.0 | 24.94 kB | 24.94 kB | | shell-quote | 1.8.3 | 23.74 kB | 23.74 kB | | limiter | 1.1.5 | 23.17 kB | 23.17 kB | | retry | 0.13.1 | 18.85 kB | 18.85 kB | | semifies | 1.0.0 | 15.84 kB | 15.84 kB | | jest-docblock | 29.7.0 | 8.99 kB | 12.76 kB | | crypto-randomuuid | 1.0.0 | 11.18 kB | 11.18 kB | | ttl-set | 1.0.0 | 4.61 kB | 9.69 kB | | mutexify | 1.4.0 | 5.71 kB | 8.74 kB | | path-to-regexp | 0.1.12 | 6.6 kB | 6.6 kB | | koalas | 1.0.2 | 6.47 kB | 6.47 kB | | module-details-from-path | 1.0.4 | 3.96 kB | 3.96 kB |

🤖 This report was automatically generated by heaviest-objects-in-the-universe

github-actions[bot] avatar Sep 19 '25 00:09 github-actions[bot]

Codecov Report

:white_check_mark: All modified and coverable lines are covered by tests. :white_check_mark: Project coverage is 83.92%. Comparing base (160ded8) to head (155afc9). :warning: Report is 4 commits behind head on master.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #6475      +/-   ##
==========================================
- Coverage   84.06%   83.92%   -0.14%     
==========================================
  Files         484      474      -10     
  Lines       20231    19907     -324     
==========================================
- Hits        17007    16707     -300     
+ Misses       3224     3200      -24     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

:rocket: New features to boost your workflow:
  • :snowflake: Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • :package: JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

codecov[bot] avatar Sep 19 '25 00:09 codecov[bot]

Benchmarks

Benchmark execution time: 2025-09-19 01:03:02

Comparing candidate commit 155afc967f52354e4fcf4e5fd0b260a56d484782 in PR branch proposal-semver-major with baseline commit 160ded801a9747b9ff5f3617790b5e21398566aa in branch master.

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

pr-commenter[bot] avatar Sep 19 '25 01:09 pr-commenter[bot]

@BridgeAR Open to other suggestions if you have any.

rochdev avatar Sep 22 '25 16:09 rochdev

What about marking the entries as deactivated or something like that?

While we should clean up our changelogs anyway. These entries could be in a separate section that make that clear.

BridgeAR avatar Sep 22 '25 17:09 BridgeAR

I guess the logic is that the PR contains a semver major change, so it should be labeled as such. The fact that the change is hidden behind a flag is an implementation detail. Then for the changelog from a user perspective, the change only exists in the new version, so it doesn't make sense to show the change in the changelog. In that sense I think it kind of all makes sense? If we do want to list every commit in changelogs though, this is no longer the case, but I'm not sure how to include them in a way that makes sense, and I'm not a fan of just flagging them as deactivated since without a clear explanation that doesn't mean much.

cc @watson as well since he had some ideas around changelogs as well.

rochdev avatar Sep 22 '25 21:09 rochdev

If the PR in question only contains code that is behind a version flag, it's safe to not include it in the changelog for older versions. However, I can easily see us getting into a situation, where a developer adds features for both version A and B and the changes in version B are breaking changes. What do we do in that case?

One solution that has a great amount of flexibility, would be to require release notes to be added to the PR body instead of relying on the PR title. That would allow the PR author to write custom release notes for each targeted release. That however, doesn't answer the question of which label to give the PR:

If a PR containing a breaking change doesn't require the major version to be bumped, because those changes are behind a version flag, then I don't think it makes sense to use the semver-major label. If we still want a label indicating this content in the PR, consider one indicating that this PR contains code which is only run on, for example, the v6.x branch (v6-only-changes?).

watson avatar Sep 24 '25 12:09 watson

@watson So you would keep semver-minor but with an added tag? Not a fan of v6-only-changes for a few reasons. First of all I don't think it's quite semantically correct as it doesn't necessarily include only changes for v6 as you mentioned in your previous comment. Also, I don't think the version number needs to be explicit, major changes will always land in the next major, so maybe something like semver-major-next? Or do you think there actually is value in being explicit on the version?

rochdev avatar Sep 29 '25 01:09 rochdev

@rochdev I'm not a fan of v6-only-changes either - it was just what I could come up with in the moment. My point was to signal that the PR contained a mixed set of changes, some of which was exclusive to the v5 branch and some exclusive to the v6 branch.

There's two scenarios we need to support with our labels:

  1. Before the release of v6, we want to be able to land a PR containing both breaking changes targeted for v6 and non-breaking changes targeted v5.
  2. After the release of v6, we want to be able to land a PR containing changes both targeted v5 and v6.

For the first scenario the label semver-major-next makes sense, but it doesn't cover the second scenario, as the word next in that case would indicate v7. So I think it would make our lives easier if we used the actual version number in the label, as that avoids any confusion about which versions are targeted with the PR.

watson avatar Sep 29 '25 09:09 watson

After the release of v6, we want to be able to land a PR containing changes both targeted v5 and v6.

Isn't that just a normal semver-patch or semver-minor?

rochdev avatar Sep 29 '25 19:09 rochdev

After the release of v6, we want to be able to land a PR containing changes both targeted v5 and v6.

Isn't that just a normal semver-patch or semver-minor?

To some extend yes, but that doesn't allow you to automatically know which release notes to update. Just the ones for v5, just the ones for v6, or both 🤷

watson avatar Oct 03 '25 07:10 watson

To some extend yes, but that doesn't allow you to automatically know which release notes to update. Just the ones for v5, just the ones for v6, or both 🤷

What I meant is that the statement After the release of v6, we want to be able to land a PR containing changes both targeted v5 and v6. means a patch or a minor. If it lands everywhere then it's not a breaking change.

Only landing on v5.x is not a thing.

Only landing on v6.x would mean it's a breaking change, in which case any label would work, hence my confusion about specifying a version or not. The only benefit that I see is that it would be easier to retroactively look at PRs and know when the change landed.

Maybe we could use milestones for this instead? I guess it's kind of meant for this purpose and it has a dedicated UI 🤔 WDYT?

Note: "landing" here means enabling the change since commits always land everywhere.

rochdev avatar Oct 05 '25 17:10 rochdev

Note: "landing" here means enabling the change since commits always land everywhere.

Understood 👍 My main concern here is also just about how we mange release notes. I'm aware that we technically merge the PR to all active release lines.

Only landing on v6.x would mean it's a breaking change.

I don't agree. Say we land a breaking change in the v6.x branch prior to the release of v6 to npm. Then if there's a bug in this breaking change after the release of v6 to npm, or if we're adding a feature to that breaking change, it's could come in a PR that only affects the v6 release. And hence, the release notes for the next v5 release shouldn't include any mention of it.

Maybe all of this is more suitable for a Zoom meeting to avoid all this back and forth 😅

watson avatar Oct 06 '25 07:10 watson

Marking as draft for the time being.

BridgeAR avatar Nov 17 '25 08:11 BridgeAR