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

feat: allow repos to be ignored by dep graph integrator #1364

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
+53 −9
Draft

feat: allow repos to be ignored by dep graph integrator #1364

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
eef8c15
feat: add functionality to ignore repos in dep graph integrator
tjsilver Dec 23, 2024
e607350
update tests
tjsilver Dec 23, 2024
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
10 changes: 10 additions & 0 deletions packages/repocop/src/config.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getDevDatabaseConfig,
} from 'common/src/database-setup';
import type { DatabaseConfig, PrismaConfig } from 'common/src/database-setup';
import type { DepGraphLanguage } from 'common/types';

export interface Config extends PrismaConfig {
/**
Expand Down Expand Up @@ -58,6 +59,11 @@ export interface Config extends PrismaConfig {
*/
dependencyGraphIntegratorTopic: string;

/**
* A list of repos to be excluded by the dependency graph integrator, by language.
Copy link
Member

Choose a reason for hiding this comment

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

Worth explicitly saying this is a repository's short name?

*/
dependencyGraphIgnoredRepos: Record<DepGraphLanguage, string[]>;

/**
* The name of the GitHub organisation that owns the repositories.
*/
Expand Down Expand Up @@ -92,6 +98,10 @@ export async function getConfig(): Promise<Config> {
dependencyGraphIntegratorTopic: getEnvOrThrow(
'DEPENDENCY_GRAPH_INPUT_TOPIC_ARN',
),
dependencyGraphIgnoredRepos: {
Scala: ['identity-platform'],
Kotlin: [],
},
gitHubOrg: process.env['GITHUB_ORG'] ?? 'guardian',
};
}
21 changes: 20 additions & 1 deletion packages/repocop/src/remediation/dependency_graph-integrator/send-to-sns.test.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,12 @@ import {

const fullName = 'guardian/repo-name';
const fullName2 = 'guardian/repo2';
const ignoredRepo = 'guardian/ignore-me';
const scalaLang = 'Scala';
const ignoredRepos = {
Scala: ['ignore-me'],
Kotlin: [],
};

function createActionsUsage(
fullName: string,
Expand Down Expand Up @@ -144,6 +149,7 @@ describe('When getting suitable events to send to SNS', () => {
[repoWithTargetLanguage(fullName)],
[repository(fullName)],
[repoWithoutWorkflow(fullName)],
ignoredRepos,
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there any reason to pass ignoredRepos here (rather than e.g. empty object)? Like the comment below, this couples these tests to the definitions more than a hundred lines away, even though they don't actually interact.

No worries if this is for consistency 👍

);
const expected = [repositoryWithDepGraphLanguage(fullName, 'Scala')];

Expand All @@ -154,6 +160,7 @@ describe('When getting suitable events to send to SNS', () => {
[repoWithTargetLanguage(fullName)],
[repository(fullName)],
[repoWithDepSubmissionWorkflow(fullName)],
ignoredRepos,
);
expect(result).toEqual([]);
});
Expand All @@ -162,14 +169,16 @@ describe('When getting suitable events to send to SNS', () => {
[repoWithoutTargetLanguage(fullName)],
[repository(fullName)],
[repoWithoutWorkflow(fullName)],
ignoredRepos,
);
expect(result).toEqual([]);
});
test('return 2 events when 2 Scala repos are found without an existing workflow', () => {
test('return 2 repos when 2 Scala repos are found without an existing workflow', () => {
const result = getReposWithoutWorkflows(
[repoWithTargetLanguage(fullName), repoWithTargetLanguage(fullName2)],
[repository(fullName), repository(fullName2)],
[repoWithoutWorkflow(fullName), repoWithoutWorkflow(fullName2)],
ignoredRepos,
);
const expected = [
repositoryWithDepGraphLanguage(fullName, 'Scala'),
Expand All @@ -178,6 +187,16 @@ describe('When getting suitable events to send to SNS', () => {

expect(result).toEqual(expected);
});
test('return empty array when an ignored Scala repo is found with without an existing workflow', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is it possible to bring the definitions for this test nearer the test body? Especially the ignoredRepo const, which I think is only used here! I really like it when tests have the setup, invocation, and assertion together, as much as possible. Where there's shared setup, we can keep that as close as possible to its use by doing that inside a describe block that only contains the tests that use it.

In this case, could we inline the ignore definitions from the top of the file here? No worries if this suggestion doesn't meet the existing style conventions!

Copy link
Contributor

@adamnfish adamnfish Dec 23, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

It's a minor point, but we aren't checking that filtering works on an array of repos that partially matches the filter - it's all or nothing at the moment. Is that worth adding? And if so, as dicsussed in the other comment, is it worth adding in a describe block that surrounds that test and this 'return empty array ... ' one so that the shared test setup of ignored repos is right here alongside its users?

describe('when repositories are ignored', () => {
  const ignoredRepo = 'guardian/ignore-me';
  const ignoredRepos = {
	Scala: ['ignore-me'],
	Kotlin: [],
  };

  test('return empty array when an ignored Scala repo is found with without an existing workflow', () => {
    // ...
  });

  test('excludes ignored repositories from the result', () => {
    // ... check the partial filter result here
  });
});

Copy link
Contributor

Choose a reason for hiding this comment

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

Also I noticed while making this example that this test name might want a look?:

return empty array when an ignored Scala repo is found with without an existing workflow

const result = getReposWithoutWorkflows(
[repoWithTargetLanguage(ignoredRepo)],
[repository(ignoredRepo)],
[repoWithoutWorkflow(ignoredRepo)],
ignoredRepos,
);

expect(result).toEqual([]);
});

const ownershipRecord1: view_repo_ownership = {
full_repo_name: fullName,
Expand Down
31 changes: 23 additions & 8 deletions packages/repocop/src/remediation/dependency_graph-integrator/send-to-sns.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ import type {
} from '@prisma/client';
import { awsClientConfig } from 'common/src/aws';
import { shuffle } from 'common/src/functions';
import { logger } from 'common/src/logs';
import type {
DependencyGraphIntegratorEvent,
DepGraphLanguage,
Expand All @@ -28,6 +29,22 @@ export function checkRepoForLanguage(
return languagesInRepo.includes(targetLanguage);
}

export function isRepoIgnored(
repoName: string,
language: DepGraphLanguage,
ignoredRepos: Record<DepGraphLanguage, string[]>,
): boolean {
const isIgnored = ignoredRepos[language].includes(repoName);
if (isIgnored) {
logger.log({
Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, great log message. This will save us confusion in the future, no doubt!

message: `Repo is being ignored by dependency graph integrator`,
depGraphLanguage: language,
repo: repoName,
});
}
return isIgnored;
}

export function doesRepoHaveDepSubmissionWorkflowForLanguage(
repo: Repository,
workflowUsagesForRepo: guardian_github_actions_usage[],
Expand Down Expand Up @@ -123,16 +140,17 @@ export function getReposWithoutWorkflows(
languages: github_languages[],
productionRepos: Repository[],
productionWorkflowUsages: guardian_github_actions_usage[],
ignoredRepos: Record<DepGraphLanguage, string[]>,
): RepositoryWithDepGraphLanguage[] {
const depGraphLanguages: DepGraphLanguage[] = ['Scala', 'Kotlin'];

const allReposWithoutWorkflows: RepositoryWithDepGraphLanguage[] =
depGraphLanguages.flatMap((language) => {
const reposWithDepGraphLanguages: Repository[] = productionRepos.filter(
(repo) => checkRepoForLanguage(repo, languages, language),
);
const reposWithDepGraphLanguages: Repository[] = productionRepos
.filter((repo) => checkRepoForLanguage(repo, languages, language))
.filter((repo) => !isRepoIgnored(repo.name, language, ignoredRepos));
console.log(
`Found ${reposWithDepGraphLanguages.length} ${language} repos in production`,
`Found ${reposWithDepGraphLanguages.length} ${language} repos in production suitable for dependency graph integration`,
);

return reposWithDepGraphLanguages
Expand All @@ -148,10 +166,6 @@ export function getReposWithoutWorkflows(
})
.map((repo) => ({ ...repo, dependency_graph_language: language }));
});

console.log(
`Found ${allReposWithoutWorkflows.length} production repos without dependency submission workflows`,
);
return allReposWithoutWorkflows;
}

Expand All @@ -169,6 +183,7 @@ export async function sendReposToDependencyGraphIntegrator(
repoLanguages,
productionRepos,
productionWorkflowUsages,
config.dependencyGraphIgnoredRepos,
);

if (reposRequiringDepGraphIntegration.length !== 0) {
Expand Down
Loading