Skip to content

Commit

Permalink
fix(ng-dev): clarify isolate primitives validation rules (#2053)
Browse files Browse the repository at this point in the history
This updates the isolate primitives validation to ensure that it is clear that once primitives code has been merged, PRs can include FW code as long as primitives code is present in the PR. In that case, those changes are related, which is fine. Pure FW PRs should not be merged in this case.

PR Close #2053
  • Loading branch information
thePunderWoman authored and josephperrott committed May 17, 2024
1 parent fca0d3f commit 9ae826d
Show file tree
Hide file tree
Showing 3 changed files with 28 additions and 3 deletions.
2 changes: 1 addition & 1 deletion .github/local-actions/branch-manager/main.js
Original file line number Diff line number Diff line change
Expand Up @@ -67445,7 +67445,7 @@ var Validation4 = class extends PullRequestValidation {
if (diffStats.separateFiles > 0 && !hasSeparateSyncFiles) {
throw this._createError(`This PR cannot be merged as Shared Primitives code has already been merged. Primitives and Framework code must be merged and synced separately. Try again after a g3sync has finished.`);
}
if (diffStats.files > 0 && hasSeparateSyncFiles) {
if (diffStats.files > 0 && diffStats.separateFiles === 0 && hasSeparateSyncFiles) {
throw this._createError(`This PR cannot be merged as Angular framework code has already been merged. Primitives and Framework code must be merged and synced separately. Try again after a g3sync has finished.`);
}
}
Expand Down
13 changes: 13 additions & 0 deletions ng-dev/pr/common/test/common.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -212,6 +212,19 @@ describe('pull request validation', () => {
const results = await assertValidPullRequest(pr, config, ngDevConfig, null, prTarget, git);
expect(results.length).toBe(0);
});

it('should allow merging when primitives changes have been merged already and PR has primitives and fw code', async () => {
const config = createIsolatedValidationConfig({assertIsolatedSeparateFiles: true});
let pr = createTestPullRequest();
const files = ['packages/core/primitives/rando.ts', 'packages/router/blah.ts'];
const fileHelper = PullRequestFiles.create(git, pr.number, googleSyncConfig);
const diffStats = {insertions: 0, deletions: 0, files: 1, separateFiles: 1, commits: 3};
spyOn(PullRequestFiles, 'create').and.returnValue(fileHelper);
spyOn(G3Stats, 'getDiffStats').and.returnValue(diffStats);
spyOn(fileHelper, 'loadPullRequestFiles').and.returnValue(Promise.resolve(files));
const results = await assertValidPullRequest(pr, config, ngDevConfig, null, prTarget, git);
expect(results.length).toBe(0);
});
});
});

Expand Down
16 changes: 14 additions & 2 deletions ng-dev/pr/common/validation/assert-isolated-separate-files.ts
Original file line number Diff line number Diff line change
Expand Up @@ -46,6 +46,8 @@ class Validation extends PullRequestValidation {
if (g3SyncConfigWithMatchers === null) {
return;
}

// diffStats tells you what's already been merged in github, but hasn't yet been synced to G3
const diffStats = await getDiffStats(config, g3SyncConfigWithMatchers.config, gitClient);
if (diffStats === undefined) {
return;
Expand All @@ -57,14 +59,24 @@ class Validation extends PullRequestValidation {
g3SyncConfigWithMatchers.config,
).pullRequestHasSeparateFiles();

// if has merged stuff already and hasSeparateSyncFiles, it's safe to merge more separate sync files
// This validation applies to PRs that get merged when changes have not yet been synced into G3.
// The rules are as follows:
// 1. if pure framework changes have been merged, separate file changes should not be merged.
// 2. if separate file changes have been merged, pure framework changes should not be merged.
// 3. if separate file changes have been merged, any change merged MUST have separate file changes in it.
// 4. framework changes can be merged with separate file changes as long as that change ALSO
// has separate file changes also.

// covers 2 & 3
if (diffStats.separateFiles > 0 && !hasSeparateSyncFiles) {
throw this._createError(
`This PR cannot be merged as Shared Primitives code has already been merged. ` +
`Primitives and Framework code must be merged and synced separately. Try again after a g3sync has finished.`,
);
}
if (diffStats.files > 0 && hasSeparateSyncFiles) {

// covers 1 & 4
if (diffStats.files > 0 && diffStats.separateFiles === 0 && hasSeparateSyncFiles) {
throw this._createError(
`This PR cannot be merged as Angular framework code has already been merged. ` +
`Primitives and Framework code must be merged and synced separately. Try again after a g3sync has finished.`,
Expand Down

0 comments on commit 9ae826d

Please sign in to comment.