Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(ng-dev): clarify isolate primitives validation rules #2053

Closed
wants to merge 1 commit into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
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 @@ -68106,7 +68106,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