Skip to content

Carry the build config key onto repository build definitions - #354

Open
melindash wants to merge 2 commits into
mage-os:mainfrom
melindash:fix/build-config-key
Open

Carry the build config key onto repository build definitions#354
melindash wants to merge 2 commits into
mage-os:mainfrom
melindash:fix/build-config-key

Conversation

@melindash

Copy link
Copy Markdown
Contributor

mergeBuildConfigs constructs each instruction with Object.assign({key}, ...), but the repositoryBuildDefinition constructor never copied options.key, so instruction.key was always undefined.

That makes the per-repository branch of the release refs lookup in mageos-release.js unreachable:

if (releaseRefs['']) instruction.ref = releaseRefs[''];
if (releaseRefs[instruction.key]) instruction.ref = releaseRefs[instruction.key];

Only the global '*' override could ever apply, so a release could not build one repository from a different ref to the rest. With the key carried through,

module.exports = {'*': 'main', magento2: 'release/3.x'};

builds magento2 from a release branch while the other 24 repositories stay on main, which avoids having to create and maintain a branch group across every repository in releaseBuildConfig.

A full release build produces identical output with and without this change when tested locally.

@melindash
melindash requested a review from a team as a code owner August 26, 2026 19:35
@rhoerr

rhoerr commented Aug 27, 2026

Copy link
Copy Markdown
Member

Confirmed — on main all 25 instructions have key === undefined, so the per-repo branch at mageos-release.js:103 was unreachable. Fix is right, suite stays green at 897.

One ask: tests/unit/utils.test.js:494 is already named should include key from b in result and its comment says it verifies the key — but the body only asserts toHaveLength(1), so it passed through the whole bug. Adding expect(result[0].key).toBe('my-repo'); there pins this at the exact seam. Two more lines in tests/unit/type/repository-build-definition.test.js (key alongside ref in the default values / provided values blocks) cover the constructor.

With those, I'd drop the new test file — its resolveRefs helper re-implements the resolution order it's testing, so it wouldn't catch a change to the real one.

Happy to merge once that lands.

melindash added a commit to melindash/generate-mirror-repo-js that referenced this pull request Aug 28, 2026
Review feedback from @rhoerr on mage-os#354.

tests/unit/utils.test.js was already named "should include key from b in result"
and its comment said it verified the key, but the body only asserted the result
length, so it passed through the whole bug. It now asserts the key.

Adds key alongside ref in the repositoryBuildDefinition constructor tests, for
both the default and provided cases.

Drops tests/unit/release-refs-targeting.test.js. Its helper re-implemented the
resolution order it was testing, so it exercised a copy of the logic rather than
the real one and would not have caught a change to it.

Verified the replacements fail without the constructor assignment: removing it
fails 2 of 136 in those two files. Suite 894.

Claude-Session: https://claude.ai/code/session_01HvYYeSXC7iC9jb81jMu2yv
Review feedback from @rhoerr on mage-os#354.

tests/unit/utils.test.js was already named "should include key from b in result"
and its comment said it verified the key, but the body only asserted the result
length, so it passed through the whole bug. It now asserts the key.

Adds key alongside ref in the repositoryBuildDefinition constructor tests, for
both the default and provided cases.

Drops tests/unit/release-refs-targeting.test.js. Its helper re-implemented the
resolution order it was testing, so it exercised a copy of the logic rather than
the real one and would not have caught a change to it.

Verified the replacements fail without the constructor assignment: removing it
fails 2 of 136 in those two files. Suite 894.

Claude-Session: https://claude.ai/code/session_01HvYYeSXC7iC9jb81jMu2yv
@melindash
melindash force-pushed the fix/build-config-key branch from 0c0e68a to 8bf8204 Compare August 28, 2026 21:52
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.

2 participants