feat: allow repos to be ignored by dep graph integrator #1364
Conversation
| ): boolean { | ||
| const isIgnored = ignoredRepos[language].includes(repoName); | ||
| if (isIgnored) { | ||
| logger.log({ |
There was a problem hiding this comment.
Ah, great log message. This will save us confusion in the future, no doubt!
| @@ -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', () => { | |||
There was a problem hiding this comment.
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!
| @@ -144,6 +149,7 @@ describe('When getting suitable events to send to SNS', () => { | |||
| [repoWithTargetLanguage(fullName)], | |||
| [repository(fullName)], | |||
| [repoWithoutWorkflow(fullName)], | |||
| ignoredRepos, | |||
There was a problem hiding this comment.
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 👍
| @@ -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', () => { | |||
There was a problem hiding this comment.
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
});
});There was a problem hiding this comment.
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
| @@ -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. | |||
There was a problem hiding this comment.
Worth explicitly saying this is a repository's short name?
What does this change?
Why?
There is at least one edge case where a repo contains a dependency graph integrator language, but that doesn't use sbt (it contains just one Scala file which is used in a script). We would like to allow teams to exclude these repos from the integrator so that it doesn't repeatedly raise PRs on those repos.
How has it been verified?