Enforce citgm for major changes

[MylesBorins]

We should do this.

At the very least it will document in the thread that we care about these breakages

[jasnell]

I'm fine with this. Would need a PR adding the requirement to the collaborator docs. If it's not there already, we should make it a required step for all releases also... perhaps even listing the failures in the release notes as known issues.

[addaleax]

Sounds good. Two thoughts:

• Reading CITGM output is hard because there are so many known/expected failures, and it’s not obvious where those come from. Can we have a tracking issue at nodejs/node with all the failures?
• While error message changes are still semver-major, can we exclude them from this rule?

[jasnell]

It makes a ton of sense to exclude certain types of semver-major's from this rule:

1. error message changes
2. semver-major changes to Node.js' own tooling, build, test framework or benchmarks

Generally, only semver-major changes in src/* and lib/* would be subject to the rule.

[sam-github]

Don't we want to know when an error message change breaks a package in npmjs.org?

[jasnell]

Sure, but those do not necessarily need to be identified before landing the PR... we're also not likely to use such a failure to determine whether or not to land such PRs. If we're changing the error message, we're likely doing so for a very good reason.

[cjihrig]

SGTM. Like @addaleax, I'd like easier to understand CITGM output.

What happens when a breaking change actually breaks something in CITGM? Would we just never break anything ever again? Is that something we'd decide on a case by case basis?

[jasnell]

We would need to decide on a case-by-case basis. CITGM is not a gate, it's an early warning system. It helps us to understand what the potential impact a change may have but it should never stop us from making any particular change.

[MylesBorins]

Fwiw the citgm team is fairly active in keeping the lookup table up to date regarding flakes. I'd be more than happy to teach people how to read the leaves, but I general failures that are not timeouts are legit failures

…

On May 11, 2017 11:12 PM, "James M Snell" ***@***.***> wrote: We would need to decide on a case-by-case basis. CITGM is not a gate, it's an early warning system. It helps us to understand what the potential impact a change may have but it should never stop us from making any particular change. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV9F2MfWvHTVIJm4IVfiOnNE_IrgCks5r43m6gaJpZM4NYH31> .

[jasnell]

Oh, I don't doubt that at all @MylesBorins. I just don't believe that failures we happen to see in citgm should block changes that we feel should be made. For example, we will never get out of the cycle of treating error message changes as semver-major unless we continue working towards assigning error codes and making the error messages more consistent -- that work could very well break existing code but the benefits far outweigh the potential disruption to existing modules.

[mhdawson]

I think we need to get CITGM to the point where it normally just runs/passes (at least one variant that we required for semver-majors). One approach may to be to fix the module versions.

Provided that the jobs run/pass reliably and takes less than 20 minutes I think requiring it for all semver majors should not be to much to ask. The issue of whether it blocks landing the PR or not can be handled on a case by case basis. Requiring the run would mean that we know where we stand earlier on as opposed to having to figure that out at release time.

[MylesBorins]

Locking the versions means we don't test the module that people get when they run npm install I cannot see any way to scalably update the lookup, especially as we add more modules. The majority of failures we see are platform related flakeyness, much of which can be specific to our infra. Seeing green citgm because we lock modules is not an accurate representation of the ecosystem

…

On May 12, 2017 9:30 AM, "Michael Dawson" ***@***.***> wrote: I think we need to get CITGM to the point where it normally just runs/passes (at least one variant that we required for semver-majors). One approach may to be to fix the module versions. Provided that the jobs run/pass reliably and takes less than 20 minutes I think requiring it for all semver majors should not be to much to ask. The issue of whether it blocks landing the PR or not can be handled on a case by case basis. Requiring the run would mean that we know where we stand earlier on as opposed to having to figure that out at release time. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#126 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAecV2L0WzURFyJeYZVutixOUSbfKd-Gks5r5F70gaJpZM4NYH31> .

[mcollina]

Currently there are a bunch of failures in citgm against master. I guess this is to be expected, as we do land breaking changes in master. How can we hold changes to have an all-green? How do I know my change did not break them more?

I have a very specific case for this, my stream.destroy() PR: nodejs/node#12925.

It would be good to have the "timeouts" reported differently than actual failures.

[MylesBorins]

@mcollina unfortunately there is no way we can do introspection into the reasoning of a modules failure, so no way to know if failures are caused by timeouts. I would argue that master would not have a bunch of failures if we gated against citgm...

[mhdawson]

Its a bit of the chicken and egg in terms of:

• we need to gate against citgm to keep citgm green
• we need citgm to be green to gate on citgm

I think we have to plot a path to getting citgm being green consistently, otherwise its too hard to get the maximum value from it. Once we managed to get the core regression builds passing most of the time, it became much easier to keep them green.

Changes in the modules themselves do add a complication and if they continuously break citgm independent from changes people are making in core we'll need a way to decouple from that. We may need both pinned modules for the regression runs for core, and the latest for nightly/releases. If we can keep citgm green on the latest all the better and we only need the one. I just think we have to get to the point were anybody can run it an it pretty much passes unless there is a problem with the change they made.

[refack]

my 4_¢

1. I actually think that CITGM should primarily be a gate for semver-minor, they should be breakage free (unless it's already so, and I just didn't RTFM).

Its a bit of the chicken and egg in terms of:

• we need to gate against citgm to keep citgm green
• we need citgm to be green to gate on citgm

2. If it's possible for the CITGM team to estimate the amount of time to get the CI job green, we call a moratorium on landing breaking changes for this time window.