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

[Test Owners] Add script to get owners for files #193277

Merged
merged 7 commits into from
Sep 20, 2024
Merged

[Test Owners] Add script to get owners for files #193277

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
9c78bf7
[Test Owners] Add script to get owners for files
wayneseymour Sep 18, 2024
e300b83
[CI] Auto-commit changed files from 'node scripts/notice'
kibanamachine Sep 18, 2024
637109f
Add fn comment to satisfy:
wayneseymour Sep 18, 2024
ac199ba
Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…
wayneseymour Sep 19, 2024
ea3b979
Add shortcircuit for files that do not exist in repo.
wayneseymour Sep 19, 2024
a7cfeb4
Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…
wayneseymour Sep 19, 2024
f98a30e
Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…
wayneseymour Sep 19, 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
6 changes: 5 additions & 1 deletion packages/kbn-code-owners/index.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,4 +8,8 @@
*/

export type { PathWithOwners } from './src/file_code_owner';
export { getPathsWithOwnersReversed, getCodeOwnersForFile } from './src/file_code_owner';
export {
getPathsWithOwnersReversed,
getCodeOwnersForFile,
runGetOwnersForFileCli,
} from './src/file_code_owner';
40 changes: 36 additions & 4 deletions packages/kbn-code-owners/src/file_code_owner.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,10 @@
*/

import { REPO_ROOT } from '@kbn/repo-info';
import { createFailError } from '@kbn/dev-cli-errors';
import { createFailError, createFlagError } from '@kbn/dev-cli-errors';
import { join as joinPath } from 'path';
import { existsSync, readFileSync } from 'fs';
import { run } from '@kbn/dev-cli-runner';

import type { Ignore } from 'ignore';
import ignore from 'ignore';
Expand All @@ -20,6 +21,10 @@ export interface PathWithOwners {
teams: string;
ignorePattern: Ignore;
}
const existOrThrow = (targetFile: string) => {
if (existsSync(targetFile) === false)
throw createFailError(`Unable to determine code owners: file ${targetFile} Not Found`);
};

/**
* Get the .github/CODEOWNERS entries, prepared for path matching.
Expand All @@ -29,9 +34,7 @@ export interface PathWithOwners {
*/
export function getPathsWithOwnersReversed(): PathWithOwners[] {
const codeownersPath = joinPath(REPO_ROOT, '.github', 'CODEOWNERS');
if (existsSync(codeownersPath) === false) {
throw createFailError(`Unable to determine code owners: file ${codeownersPath} not found`);
}
existOrThrow(codeownersPath);
const codeownersContent = readFileSync(codeownersPath, { encoding: 'utf8', flag: 'r' });
const codeownersLines = codeownersContent.split(/\r?\n/);
const codeowners = codeownersLines
Expand Down Expand Up @@ -66,3 +69,32 @@ export function getCodeOwnersForFile(

return match?.teams;
}

/**
* Run the getCodeOwnersForFile() method above.
* Report back to the cli with either success and the owner(s), or a failure.
*
* This function depends on a --file param being passed on the cli, like this:
* $ node scripts/get_owners_for_file.js --file SOME-FILE
*/
export async function runGetOwnersForFileCli() {
run(
async ({ flags, log }) => {
const targetFile = flags.file as string;
if (!targetFile) throw createFlagError(`Missing --flag argument`);
Comment on lines +83 to +84
Copy link
Member

Choose a reason for hiding this comment

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

Should we check if provided argument is actually a valid existing path?

Did some testing and I would expect error pointing the path is incorrect for the last 2 calls:

[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/modes/index.ts
 succ elastic/kibana-management
[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/modes
 succ elastic/kibana-management
[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/mode
 succ elastic/kibana-management
[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/x
 succ elastic/kibana-management

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrmm, as I understand it, the codewoners resolution is essentially looking for files and folders.
Such that, if a file is not found, the result can be a parent dir (depending how the ownership is declared [specific file vs folder]). This is my guess on the algo; that's how I do it manually.

That said, $ node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/x:

 succ elastic/kibana-management

...gives me pause.

Lemme see what I can discover.

Good catch mate!

Copy link
Member Author

Choose a reason for hiding this comment

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

FIx?

Sorry about the force push. I was in a meeting showing someone something and ran a rebase in the wrong dir. Wont happen again (after opening pr's for review)

Copy link
Member

Choose a reason for hiding this comment

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

no worries, checking...

Copy link
Contributor

Choose a reason for hiding this comment

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

The error message seems to be off. It mentions a --flag argument, that's not present in the args.

existOrThrow(targetFile); // This call is duplicated in getPathsWithOwnersReversed(), so this is a short circuit
const result = getCodeOwnersForFile(targetFile);
if (result) log.success(result);
else log.error(`Ownership of file [${targetFile}] is UNKNOWN`);
},
{
description: 'Report file ownership from GitHub CODEOWNERS file.',
flags: {
string: ['file'],
Copy link
Member Author

Choose a reason for hiding this comment

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

@delanni is this not it bud?

Copy link
Member Author

Choose a reason for hiding this comment

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

@delanni Haven't heard from ya man. Gonna merge this soon. Please lemme know if you've an objection

Copy link
Contributor

Choose a reason for hiding this comment

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

Sorry, I didn't see the message.
I meant the wording of the error:

throw createFlagError(`Missing --flag argument`);

but there's no --flag argument, it's --file

Copy link
Member Author

Choose a reason for hiding this comment

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

Ahhhhh!!!!

Copy link
Member Author

Choose a reason for hiding this comment

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

#193581

@delanni

help: `
--file Required, path to the file to report owners for.
`,
},
}
);
}
3 changes: 2 additions & 1 deletion packages/kbn-code-owners/tsconfig.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@
],
"kbn_references": [
"@kbn/repo-info",
"@kbn/dev-cli-errors"
"@kbn/dev-cli-errors",
"@kbn/dev-cli-runner"
]
}
11 changes: 11 additions & 0 deletions scripts/get_owners_for_file.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
/*
* Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one
* or more contributor license agreements. Licensed under the "Elastic License
* 2.0", the "GNU Affero General Public License v3.0 only", and the "Server Side
* Public License v 1"; you may not use this file except in compliance with, at
* your election, the "Elastic License 2.0", the "GNU Affero General Public
* License v3.0 only", or the "Server Side Public License, v 1".
*/

require('../src/setup_node_env');
require('@kbn/code-owners').runGetOwnersForFileCli();