diff --git a/docs/contributing.md b/docs/contributing.md index 84329477..9ec86269 100644 --- a/docs/contributing.md +++ b/docs/contributing.md @@ -255,20 +255,27 @@ If there are no changelog entries to add, remove the code block and start the ch The version-compat job in CI runs the local controller with the latest published host, and the published controller with the local host. It fails if a link message changed its src, dst, data or response schema since that release. -If the break is intended, add an entry for the published version to [scripts/version-compat-breaks.json](../scripts/version-compat-breaks.json) in the same pull request: +If the break is intended, add an entry to [scripts/version-compat-breaks.json](../scripts/version-compat-breaks.json) in the same pull request. +Breaking changes are only accepted if the next release is intended to include them, see our decision on [breaking changes](/docs/decisions.md#breaking-changes). +The entry is keyed by the published version directly before the breaking change was introduced, which is the latest release at the time. +Every entry needs the pull request number, a reason saying what breaks, and an impact saying why end users are not or only minimally affected. ```json { "2.0.3": { "messages": { - "request FactorioVersionsRequest": { "pr": 1129, "reason": "Why it breaks and what stops working." } + "request FactorioVersionsRequest": { + "pr": 1129, + "reason": "What breaks between the two versions.", + "impact": "Why end users are not or only minimally affected." + } }, - "connect": { "pr": 1234, "reason": "Why a host cannot connect at all." } + "connect": { "pr": 1234, "reason": "Why a host cannot connect at all.", "impact": "..." } } } ``` -Listed messages are reported as warnings with the reason instead of failing the job. +Listed messages are reported as warnings with the reason and impact instead of failing the job. Only add `connect` when the published and local versions cannot connect at all, it skips starting the cluster. Listing a message that did not change fails the job. Entries only apply while their version is the latest release, after the next release they are ignored and can be removed. diff --git a/scripts/version-compat-breaks.js b/scripts/version-compat-breaks.js index 177c33c4..eba160df 100644 --- a/scripts/version-compat-breaks.js +++ b/scripts/version-compat-breaks.js @@ -7,9 +7,9 @@ * { * "2.0.3": { * "messages": { - * "request FactorioVersionsRequest": { "pr": 1129, "reason": "..." } + * "request FactorioVersionsRequest": { "pr": 1129, "reason": "...", "impact": "..." } * }, - * "connect": { "pr": 1234, "reason": "..." } + * "connect": { "pr": 1234, "reason": "...", "impact": "..." } * } * } * @@ -30,10 +30,12 @@ function validateBreak(value, where) { if (!Number.isInteger(value.pr) || value.pr <= 0) { throw new Error(`${where} must have a pr number`); } - if (typeof value.reason !== "string" || !value.reason.trim()) { - throw new Error(`${where} must have a reason`); + for (const key of ["reason", "impact"]) { + if (typeof value[key] !== "string" || !value[key].trim()) { + throw new Error(`${where} is missing ${key}`); + } } - const unknown = Object.keys(value).filter(key => !["pr", "reason"].includes(key)); + const unknown = Object.keys(value).filter(key => !["pr", "reason", "impact"].includes(key)); if (unknown.length) { throw new Error(`${where} has unknown properties ${unknown.join(", ")}`); } diff --git a/scripts/version-compat-breaks.json b/scripts/version-compat-breaks.json index 5e0b02ca..13bd5997 100644 --- a/scripts/version-compat-breaks.json +++ b/scripts/version-compat-breaks.json @@ -3,11 +3,13 @@ "messages": { "request FactorioVersionsRequest": { "pr": 1129, - "reason": "Hosts send it while starting an instance, so a newer host cannot start instances on a 2.0.3 controller." + "reason": "Hosts send it while starting an instance, which a 2.0.3 controller rejects so the start fails.", + "impact": "The host stays connected and the instance goes back to stopped with the error shown, starting it works again once the controller is updated." }, "request LatestReleasesRequest": { "pr": 1129, - "reason": "Hosts send it while starting an instance that targets a release channel such as stable, which a 2.0.3 controller rejects." + "reason": "Hosts send it while starting an instance that targets a release channel such as stable.", + "impact": "Hosts send FactorioVersionsRequest first, so a 2.0.3 controller rejects the start before this is sent, starting it works again once the controller is updated." } } } diff --git a/scripts/version-compat-test.js b/scripts/version-compat-test.js index e888d9b1..b300833e 100644 --- a/scripts/version-compat-test.js +++ b/scripts/version-compat-test.js @@ -247,8 +247,8 @@ async function checkMessageCompat(publishedVersion, knownBreaks) { const { known, unknown, stale } = matchBreaks(knownBreaks, changed.map(({ name }) => name)); for (const { name, fields, localSpec, publishedSpec } of changed) { if (known.includes(name)) { - const { pr, reason } = knownBreaks.get(name); - annotate(`Link ${name} is a known break since ${publishedVersion} from #${pr}: ${reason}`); + const { pr, reason, impact } = knownBreaks.get(name); + annotate(`Link ${name} is a known break since ${publishedVersion} from #${pr}: ${reason} Impact: ${impact}`); printChange(console.log, name, fields, localSpec, publishedSpec, publishedVersion); } else { printChange(console.error, name, fields, localSpec, publishedSpec, publishedVersion); @@ -308,8 +308,10 @@ async function main() { } await checkMessageCompat(publishedLib.version, knownBreaks.messages); if (knownBreaks.connect) { - const { pr, reason } = knownBreaks.connect; - annotate(`Skipping cluster test, known break since ${publishedLib.version} from #${pr}: ${reason}`); + const { pr, reason, impact } = knownBreaks.connect; + annotate( + `Skipping cluster test, known break since ${publishedLib.version} from #${pr}: ${reason} Impact: ${impact}` + ); console.log(`PASS: link messages match ${publishedLib.version} apart from known breaks`); return; } diff --git a/test/version_compat_breaks.js b/test/version_compat_breaks.js index 21c42a48..f273bc2c 100644 --- a/test/version_compat_breaks.js +++ b/test/version_compat_breaks.js @@ -5,17 +5,17 @@ import { } from "../scripts/version-compat-breaks.js"; -describe("version compatibility known breaks", function() { +describe("scripts/version-compat-breaks", function() { const breaks = { "2.0.3": { messages: { - "request ListedRequest": { pr: 1, reason: "Listed." }, - "event UnchangedEvent": { pr: 2, reason: "Unchanged." }, + "request ListedRequest": { pr: 1, reason: "Listed.", impact: "None." }, + "event UnchangedEvent": { pr: 2, reason: "Unchanged.", impact: "None." }, }, - connect: { pr: 3, reason: "Cannot connect." }, + connect: { pr: 3, reason: "Cannot connect.", impact: "None." }, }, "2.0.2": { - messages: { "request OldRequest": { pr: 4, reason: "Old." } }, + messages: { "request OldRequest": { pr: 4, reason: "Old.", impact: "None." } }, }, }; @@ -27,24 +27,30 @@ describe("version compatibility known breaks", function() { validateBreaks(breaks); validateBreaks({}); }); - it("should require a pr number and reason", function() { + it("should require a pr number, reason and impact", function() { assert.throws( () => validateBreaks({ "1.0.0": { messages: { "request A": { reason: "A." } } } }), /must have a pr number/ ); assert.throws( () => validateBreaks({ "1.0.0": { messages: { "request A": { pr: 1, reason: " " } } } }), - /must have a reason/ + /is missing reason/ + ); + assert.throws( + () => validateBreaks({ "1.0.0": { messages: { "request A": { pr: 1, reason: "A." } } } }), + /is missing impact/ ); assert.throws( () => validateBreaks({ "1.0.0": { connect: { pr: 1 } } }), - /connect must have a reason/ + /connect is missing reason/ ); }); it("should reject unknown properties and bad message names", function() { assert.throws(() => validateBreaks({ "1.0.0": { connected: false } }), /unknown properties connected/); assert.throws( - () => validateBreaks({ "1.0.0": { messages: { "request A": { pr: 1, reason: "A.", x: 1 } } } }), + () => validateBreaks({ + "1.0.0": { messages: { "request A": { pr: 1, reason: "A.", impact: "A.", x: 1 } } }, + }), /unknown properties x/ ); assert.throws( @@ -58,7 +64,7 @@ describe("version compatibility known breaks", function() { it("should pick the entry for the published version", function() { const selected = selectBreaks(breaks, "2.0.3"); assert.deepEqual([...selected.messages.keys()], ["request ListedRequest", "event UnchangedEvent"]); - assert.deepEqual(selected.connect, { pr: 3, reason: "Cannot connect." }); + assert.deepEqual(selected.connect, { pr: 3, reason: "Cannot connect.", impact: "None." }); assert.deepEqual(selected.ignored, ["2.0.2"]); }); it("should ignore every entry when the version is not listed", function() {