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

wayneseymour · 2024-09-18T11:51:49Z

Summary

Node script to report ownership of a given file in our repo.

The script's source of truth is .github/CODEOWNERS, which is generated by @kbn/generate

In order to reach the goal of have zero files without code ownership, this is one small step along the way.

To Test

Happy Path

node scripts/get_owners_for_file.js --file packages/kbn-ace/src/ace/modes/index.ts

 succ elastic/kibana-management

Unknown Path

node scripts/get_owners_for_file.js --file some-file.txt

ERROR Ownership of file [some-file.txt] is UNKNOWN

Error Path

node scripts/get_owners_for_file.js

ERROR Missing --flag argument

  node scripts/get_owners_for_file.js

  Report file ownership from GitHub CODEOWNERS file.

  Options:
    --file             Required, path to the file to report owners for.
    --verbose, -v      Log verbosely
    --debug            Log debug messages (less than verbose)
    --quiet            Only log errors
    --silent           Don't log anything
    --help             Show this message

Notes

Along with this small pr, next will be to ensure owners are assigned to all ES and KBN Archives. See more info in the link below:

Contributes to: #192979

wayneseymour · 2024-09-18T11:55:25Z

/ci

Contributes to: elastic#192979

wayneseymour · 2024-09-18T12:43:26Z

/ci

"node scripts/build_api_docs --plugin @kbn/code-owners --stats comments"

wayneseymour · 2024-09-18T14:43:43Z

@elasticmachine merge upstream

dmlemeshko · 2024-09-19T09:07:56Z

packages/kbn-code-owners/src/file_code_owner.ts

+      const targetFile = flags.file as string;
+      if (!targetFile) throw createFlagError(`Missing --flag argument`);


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

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!

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)

no worries, checking...

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

dmlemeshko

Great to see the new script, left a question about path validation

…files

dmlemeshko

LGTM.

general question:

[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file x-pack/test/api_integration/deployment_agnostic
ERROR Ownership of file [x-pack/test/api_integration/deployment_agnostic] is UNKNOWN

[get-owners-for-files][~/github/kibana]$ node scripts/get_owners_for_file.js --file x-pack/test/api_integration/deployment_agnostic/default_configs
 succ elastic/appex-qa,#temporarily,to,monitor,tests,migration

It seems to be fine that "root" test directory has no owners as long as all of it child folders (or their child folders, etc...) have owners. Do I see it the right way?

x-pack/test/api_integration/deployment_agnostic may not have explicit owner, but
x-pack/test/api_integration/deployment_agnostic/default_configs
x-pack/test/api_integration/deployment_agnostic/services
...
should either have one, or their files should. wdyt?

wayneseymour · 2024-09-19T11:37:50Z

LGTM.

It seems to be fine that "root" test directory has no owners as long as all of it child folders (or their child folders, etc...) have owners. Do I see it the right way?

x-pack/test/api_integration/deployment_agnostic may not have explicit owner, but x-pack/test/api_integration/deployment_agnostic/default_configs x-pack/test/api_integration/deployment_agnostic/services ... should either have one, or their files should. wdyt?

Yes I believe that is correct. We will know more once we get closer to having zero files/folders without ownership in test, x-pack/test && x-pack/test_serverless

wayneseymour · 2024-09-19T12:32:42Z

Force pushed again to see if I can stop the auto-commit which kicked off a ton of extra tests that have nothing to do with my pr

…files

kibana-ci · 2024-09-19T16:45:27Z

💚 Build Succeeded

Buildkite Build
Commit: f98a30e

Metrics [docs]

Unknown metric groups

API count

id	before	after	diff
`@kbn/code-owners`	8	9	+1

History

💔 Build #235572 failed ea3b979
💔 Build #235548 failed b0d89f33452b88eef71fce2ae07f26c559899a9b
💚 Build #235274 succeeded 5731de1002db190192f490c1314636eace214d23
💛 Build #235210 was flaky e300b83
💔 Build #235185 failed 63611e632dcfab305108a0587a2919723549b27e

To update your PR or re-run it, just comment with:
@elasticmachine merge upstream

cc @wayneseymour

wayneseymour · 2024-09-20T07:57:00Z

packages/kbn-code-owners/src/file_code_owner.ts

+    {
+      description: 'Report file ownership from GitHub CODEOWNERS file.',
+      flags: {
+        string: ['file'],


@delanni is this not it bud?

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

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

#193581

@delanni

## Summary Modify code owner declarations for @elastic/response-ops team in `.github/CODEOWNERS` ### Update for Reviewers With the merge of [this](#193277), you can now do this: `node scripts/get_owners_for_file.js --file x-pack/test/functional/es_archives/action_task_params` ``` succ elastic/response-ops ``` _So, you can use this script to see if the code owners file is reporting erroneously_ ### For Reviewers Added these lines: ``` /x-pack/test/functional/es_archives/actions @elastic/response-ops /x-pack/test/functional/es_archives/alerting @elastic/response-ops /x-pack/test/functional/es_archives/alerts @elastic/response-ops /x-pack/test/functional/es_archives/alerts_legacy @elastic/response-ops /x-pack/test/functional/es_archives/observability/alerts @elastic/response-ops /x-pack/test/functional/es_archives/actions @elastic/response-ops /x-pack/test/functional/es_archives/rules_scheduled_task_id @elastic/response-ops /x-pack/test/functional/es_archives/alerting/8_2_0 @elastic/response-ops ``` Most of these changes come from searching for `esArchiver.load`, within `x-pack/test/alerting_api_integration` Obviously this can easily be erroneous, so please keep me honest. #### Notes This is a best guess effort, as we march towards having zero files within `test` && `x-pack/test` && `x-pack/test_serverless` without a code owner. In the end, this will contribute to better reporting. Contribues to: #192979 --------- Co-authored-by: kibanamachine <[email protected]> Co-authored-by: Elastic Machine <[email protected]>

## Summary Follow up of [incorrect wording](#193277 (comment))

#193571) ## Summary Modify code owner declarations for @elastic/security-detection-engine team in `.github/CODEOWNERS` ### Update for Reviewers With the merge of [this](#193277), you can now do this: `node scripts/get_owners_for_file.js --file x-pack/test/functional/es_archives/asset_criticality` ``` succ elastic/security-detection-engine ``` _So, you can use this script to see if the code owners file is reporting erroneously_ ### Only one un-contested change: 1. `/x-pack/test/functional/es_archives/asset_criticality @elastic/security-detection-engine` ### Note Oringinally I included `/x-pack/test/functional/es_archives/security_solution @elastic/security-detection-engine` but this was contested. We will circle back around to this. Contributes to: #192979 --------- Co-authored-by: Elastic Machine <[email protected]>

wayneseymour added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting FTR labels Sep 18, 2024

wayneseymour self-assigned this Sep 18, 2024

wayneseymour requested a review from a team as a code owner September 18, 2024 11:51

wayneseymour mentioned this pull request Sep 18, 2024

[FTR][No Missing Owners for Test Files] Enable Code Owners Check on PR's #192979

Closed

6 tasks

wayneseymour marked this pull request as draft September 18, 2024 11:54

wayneseymour and others added 2 commits September 18, 2024 13:42

[Test Owners] Add script to get owners for files

9c78bf7

Contributes to: elastic#192979

[CI] Auto-commit changed files from 'node scripts/notice'

e300b83

wayneseymour force-pushed the get-owners-for-files branch from d0d9d56 to e300b83 Compare September 18, 2024 12:42

Add fn comment to satisfy:

637109f

"node scripts/build_api_docs --plugin @kbn/code-owners --stats comments"

wayneseymour marked this pull request as ready for review September 18, 2024 14:43

dmlemeshko reviewed Sep 19, 2024

View reviewed changes

wayneseymour added 2 commits September 19, 2024 11:43

Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…

ac199ba

…files

Add shortcircuit for files that do not exist in repo.

ea3b979

wayneseymour force-pushed the get-owners-for-files branch from 5731de1 to ea3b979 Compare September 19, 2024 11:01

wayneseymour requested a review from dmlemeshko September 19, 2024 11:06

dmlemeshko approved these changes Sep 19, 2024

View reviewed changes

elastic-vault-github-plugin-prod bot requested a review from a team as a code owner September 19, 2024 11:47

wayneseymour force-pushed the get-owners-for-files branch from b0d89f3 to ea3b979 Compare September 19, 2024 12:32

wayneseymour added 2 commits September 19, 2024 14:55

Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…

a7cfeb4

…files

Merge branch 'main' of github.com:elastic/kibana into get-owners-for-…

f98a30e

…files

PhilippeOberti removed the request for review from a team September 19, 2024 21:52

wayneseymour commented Sep 20, 2024

View reviewed changes

wayneseymour requested a review from delanni September 20, 2024 12:55

wayneseymour merged commit 8a79173 into elastic:main Sep 20, 2024
21 checks passed

kibanamachine added the v9.0.0 label Sep 20, 2024

This was referenced Sep 20, 2024

[FTR][Ownership] add @elastic/security-detection-engine to es archives #193571

Merged

[FTR][Ownership] add @elastic/response-ops to es archives #193449

Merged

[FTR] Fixup err msg for scripts/get_owners_for_file.js #193581

Merged

wayneseymour added a commit that referenced this pull request Sep 20, 2024

[FTR] Fixup err msg for scripts/get_owners_for_file.js (#193581)

4172226

## Summary Follow up of [incorrect wording](#193277 (comment))

wayneseymour mentioned this pull request Dec 3, 2024

[Ownership] Print owner match also, not just owner #202666

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

wayneseymour commented Sep 18, 2024 •

edited

Loading

wayneseymour commented Sep 18, 2024

wayneseymour commented Sep 18, 2024

wayneseymour commented Sep 18, 2024

dmlemeshko Sep 19, 2024

wayneseymour Sep 19, 2024

wayneseymour Sep 19, 2024

dmlemeshko Sep 19, 2024

delanni Sep 20, 2024

dmlemeshko left a comment

dmlemeshko left a comment •

edited

Loading

wayneseymour commented Sep 19, 2024

wayneseymour commented Sep 19, 2024

kibana-ci commented Sep 19, 2024

API count

wayneseymour Sep 20, 2024

wayneseymour Sep 20, 2024

delanni Sep 20, 2024

wayneseymour Sep 20, 2024

wayneseymour Sep 20, 2024

		const targetFile = flags.file as string;
		if (!targetFile) throw createFlagError(`Missing --flag argument`);

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

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

Conversation

wayneseymour commented Sep 18, 2024 • edited Loading

Summary

To Test

Happy Path

Unknown Path

Error Path

Notes

wayneseymour commented Sep 18, 2024

wayneseymour commented Sep 18, 2024

wayneseymour commented Sep 18, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dmlemeshko left a comment

Choose a reason for hiding this comment

dmlemeshko left a comment • edited Loading

Choose a reason for hiding this comment

wayneseymour commented Sep 19, 2024

wayneseymour commented Sep 19, 2024

kibana-ci commented Sep 19, 2024

💚 Build Succeeded

Metrics [docs]

API count

History

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wayneseymour commented Sep 18, 2024 •

edited

Loading

dmlemeshko left a comment •

edited

Loading