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.

Sign up for GitHub

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

chore: add PR to R2 list only if there are 2 approvals #33283

Merged
merged 5 commits into from
Feb 6, 2025
+54 −46
Merged

chore: add PR to R2 list only if there are 2 approvals #33283

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0784c2e
add to r2 list only if approval count is 2 or more
samson-keung Feb 4, 2025
dfc08a0
revert trigger change
samson-keung Feb 4, 2025
68b03d8
update unit tests
samson-keung Feb 4, 2025
e9e870f
Merge branch 'main' into listen-to-dequeue-for-r2-list
mergify[bot] Feb 5, 2025
e16c0fc
Merge branch 'main' into listen-to-dequeue-for-r2-list
mergify[bot] Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
58 changes: 34 additions & 24 deletions scripts/@aws-cdk/script-tests/prioritization/assign-r2-priority.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const { PRIORITIES, LABELS, STATUS, ...PROJECT_CONFIG } = require('../../../../scripts/prioritization/project-config');
const {
const {
createMockGithubForR2,
OPTION_IDS
} = require('./helpers/mock-data');
Expand All @@ -8,47 +8,47 @@ const assignR2Priority = require('../../../../scripts/prioritization/assign-r2-p

describe('Priority Assignment (R2)', () => {
let mockGithub;

beforeEach(() => {
jest.clearAllMocks();
});

async function verifyProjectState(expectedPriority, expectedStatus) {
const calls = mockGithub.graphql.mock.calls;

if (!expectedPriority) {
const priorityUpdateCall = calls.find(call =>
const priorityUpdateCall = calls.find(call =>
call[1].input?.fieldId === PROJECT_CONFIG.priorityFieldId
);
expect(priorityUpdateCall).toBeUndefined();
return;
}

// Get the existing project item data from the mock response
const projectItemResponse = await mockGithub.graphql.mock.results[2].value;
const existingPRData = projectItemResponse.node.projectItems.nodes[0];

// Verify priority update
const priorityUpdateCall = calls.find(call =>
const priorityUpdateCall = calls.find(call =>
call[1].input?.fieldId === PROJECT_CONFIG.priorityFieldId
);
expect(priorityUpdateCall[1].input.value.singleSelectOptionId)
.toBe(OPTION_IDS[expectedPriority]);

// Find any status update calls
const statusUpdateCall = calls.find(call =>
const statusUpdateCall = calls.find(call =>
call[1].input?.fieldId === PROJECT_CONFIG.statusFieldId
);

if (existingPRData) {
// For existing PR
// Get the existing status
const existingStatus = existingPRData.fieldValues.nodes
.find(node => node.field.name === 'Status')?.name;

// Verify no status update was made
expect(statusUpdateCall).toBeUndefined();

// Verify expected status matches existing status
expect(existingStatus).toBe(expectedStatus);
} else {
Expand All @@ -61,53 +61,63 @@ describe('Priority Assignment (R2)', () => {
describe('R2 Priority Tests', () => {
test('should assign R2 priority and Ready status to approved PR with failing checks', async () => {
mockGithub = createMockGithubForR2({
approved: true,
approvalCount: 2,
checksState: 'FAILURE'
});

await assignR2Priority({ github: mockGithub });
await verifyProjectState(PRIORITIES.R2, STATUS.READY);
});

test('should not assign R2 priority to PR without approval', async () => {
mockGithub = createMockGithubForR2({
approved: false,
checksState: 'FAILURE'
approvalCount: 0,
checksState: 'SUCCESS'
});

await assignR2Priority({ github: mockGithub });
await verifyProjectState(null);
});

test('should not assign R2 priority to PR with passing checks', async () => {
test('should not assign R2 priority to PR with a single approval', async () => {
mockGithub = createMockGithubForR2({
approved: true,
approvalCount: 1,
checksState: 'SUCCESS'
});


await assignR2Priority({ github: mockGithub });
await verifyProjectState(null);
});

test('should assign R2 priority to PR with passing checks', async () => {
mockGithub = createMockGithubForR2({
approvalCount: 2,
checksState: 'SUCCESS'
});

await assignR2Priority({ github: mockGithub });
await verifyProjectState(null);
});

test('should update existing PR to R2 priority', async () => {
mockGithub = createMockGithubForR2({
approved: true,
approvalCount: 2,
checksState: 'FAILURE',
existingPriority: PRIORITIES.R3,
existingStatus: STATUS.IN_PROGRESS
});

await assignR2Priority({ github: mockGithub });
await verifyProjectState(PRIORITIES.R2, STATUS.IN_PROGRESS);
});

test('should not update if PR already has R2 priority', async () => {
mockGithub = createMockGithubForR2({
approved: true,
approvalCount: 2,
checksState: 'FAILURE',
existingPriority: PRIORITIES.R2
});

await assignR2Priority({ github: mockGithub });
await verifyProjectState(null);
});
Expand Down
16 changes: 7 additions & 9 deletions scripts/@aws-cdk/script-tests/prioritization/helpers/mock-data.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -101,11 +101,11 @@ exports.createMockGithub = () => {
/**
* Creates mock GitHub GraphQL client with predefined responses for R5 priority
*/
exports.createMockGithubForR5 = ({
draft = false,
labels = [],
exports.createMockGithubForR5 = ({
draft = false,
labels = [],
updatedAt = new Date(Date.now() - 22 * 24 * 60 * 60 * 1000).toISOString(),
existingPriority = null
existingPriority = null
}) => {
const graphql = jest.fn();

Expand Down Expand Up @@ -167,8 +167,8 @@ exports.createMockGithubForR5 = ({
/**
* Creates mock GitHub GraphQL client with predefined responses for R2 priority
*/
exports.createMockGithubForR2 = ({
approved = false,
exports.createMockGithubForR2 = ({
approvalCount = 0,
checksState = 'SUCCESS',
existingPriority = null,
existingStatus = STATUS.READY
Expand All @@ -186,9 +186,7 @@ exports.createMockGithubForR2 = ({
id: 'PR_123',
number: 123,
reviews: {
nodes: approved ? [
{ state: 'APPROVED' }
] : []
nodes: Array.from({ length: approvalCount }, () => ({ state: 'APPROVED' }))
},
commits: {
nodes: [{
Expand Down
26 changes: 13 additions & 13 deletions scripts/prioritization/assign-r2-priority.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,10 +41,10 @@ module.exports = async ({ github }) => {
console.log(`Total PRs fetched: ${allPRs.length}`);

// Get project fields
const projectFields = await fetchProjectFields({
github,
const projectFields = await fetchProjectFields({
github,
org: PROJECT_CONFIG.org,
number: PROJECT_CONFIG.projectNumber
number: PROJECT_CONFIG.projectNumber
});

const priorityField = projectFields.organization.projectV2.fields.nodes.find(
Expand All @@ -65,27 +65,27 @@ module.exports = async ({ github }) => {

for (const pr of allPRs) {
try {

console.log(`Processing PR #${pr.number}....`);

// Check PR status
const isApproved = pr.reviews.nodes.some(
const approvals = pr.reviews.nodes.filter(
(review) => review.state === "APPROVED"
);

// Skip if PR is not approved
if (!isApproved) {
if (approvals.length < 2) {
console.log(`Observed approval count: ${approvals.length}. Skipping as it is less than 2...`);
Comment on lines +75 to +76
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we make it R3 if it only has one approval? Often I think if one community member is reviewing a PR then the others will leave it alone and wait for maintainer

Comment on lines +75 to +76
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm thinking should we consider checking approval based on a maintainer list instead of just counting approvals? This would prevent PRs from being added to R2 when they have multiple community approvals but no maintainer approval with failing checks.

We could use an approach similar to mergify configuration where:

  1. Maintain maintainer authors list in one common place
  2. Check if approval is from someone in this list
  3. Only add to R2 if a maintainer has approved and checks are failing

For approved PR's from community and needs maintainer review with failing status check will be added to R3 list if I'm not wrong.

Let me know if it makes sense.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thought about the same but as I went through a few PRs (very few as I was manually going through them), counting 2 approvals should cover most cases. Because of that, I didn't want to make this too complicated.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for checking. Should be good then if most of the PRs falls under this behaviour.

continue;
}

// Check status of checks
const checksState = pr.commits.nodes[0]?.commit.statusCheckRollup?.state;
const checksNotPassing = checksState !== "SUCCESS";

// Skip if PR checks is not passing
if (!checksNotPassing) {
console.log(`PR checks are failing. Skipping...`);
continue;
}

console.log(`Processing PR #${pr.number} for ${PRIORITIES.R2} priority consideration`);

// Get all projects the PR added to
const result = await fetchProjectItem({
Expand All @@ -96,17 +96,17 @@ module.exports = async ({ github }) => {
// Filter our specific project
const projectItem = result.node.projectItems.nodes
.find(item => item.project.id === PROJECT_CONFIG.projectId);

if (projectItem) {
// PR already in project
const currentPriority = projectItem.fieldValues.nodes
.find(fv => fv.field?.name === 'Priority')?.name;

if (currentPriority === PRIORITIES.R2) {
console.log(`PR #${pr.number} already has ${PRIORITIES.R2} priority. Skipping.`);
continue;
}

// Update priority only, maintain existing status
console.log(`Updating PR #${pr.number} from ${currentPriority} to ${PRIORITIES.R2} priority`);
await updateProjectField({
Expand Down
Loading