-
Notifications
You must be signed in to change notification settings - Fork 8.3k
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.
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
[FTR][No Missing Owners for Test Files] Enable Code Owners Check on PR's #192979
Comments
Pinging @elastic/appex-qa (Team:QA) |
Great idea! Just few questions:
What are we trying to solve here? Tests and services are being constantly changed and usually scoped to particular team. I see the point of keeping code owners. Archives are generated one time and widely shared by different teams in quite different tests scenarios. It is gzip files and code review is problematic. But as long as all tests pass on CI, we are good. Thoughts?
Just an idea: for the start we can enable CI check as opt-in by adding label to the PR and check some PRs without actually blocking them e.g.
See |
I think the idea here is that first I've to ensure all archives have owners, which will include assigning any that do not.
I concur! 💯 |
|
Contributes to: elastic#192979
Contributes to: elastic#192979
## 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 --------- Co-authored-by: kibanamachine <[email protected]>
## 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]>
#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]>
…#194412) ## Summary Modify code owner declarations for `x-pack/test/functional/es_archives/canvas/logstash_lens` in .github/CODEOWNERS ### For reviewers To verify this pr, you can use the `scripts/get_owners_for_file.js` script E.g: ``` node scripts/get_owners_for_file.js --file x-pack/test/functional/es_archives/canvas/logstash_lens # Or any other file ``` #### Notes All of these are a best guess effort. The more help from the dev teams, the more accurate this will be for reporting in the future. Contributes to: #192979 --------- Co-authored-by: kibanamachine <[email protected]>
…stic#200147) ## Summary Assign test files to observability-ai-assistant team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: kibanamachine <[email protected]>
## Summary Assign test files to viz team Contributes to: elastic#192979
…200937) ## Summary Assign test files to obs-ux-infra_services-team Contributes to: elastic#192979
## Summary Assign test files to observability-ui team Contributes to: elastic#192979
## Summary Assign test files to fleet team Contributes to: elastic#192979
## Summary Assign test files to presentation team Contributes to: elastic#192979
## Summary Assign test files to response ops team Contributes to: elastic#192979
## Summary Assign test files to qa team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
…#201003) ## Summary Assign test files to cloud-security-posture team Contributes to: elastic#192979
## Summary Assign test files to platform-security team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to obs-ux-logs team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
…tic#201000) ## Summary Assign test files to security-defend-workflows team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to core team Contributes to: elastic#192979
## Summary Assign test files to viz team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to search team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to obs ai assist team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign more to presentation team Contributes to: elastic#192979
## Summary Assign test files to mgmt team Contributes to: elastic#192979
## Summary Assign test files to data discovery team Contributes to: elastic#192979
## Summary Assign test files to logstash team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to ml team Contributes to: elastic#192979
…lastic#200998) ## Summary Assign test files to security-detections-response team Contributes to: elastic#192979 Co-authored-by: Elastic Machine <[email protected]>
) ## Summary Assign test screenshots to presentation team Contributes to: elastic#192979
## Summary Assign test files to data discovery team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Julia Rechkunova <[email protected]>
## Summary Assign test files to fleet team Contributes to: elastic#192979
## Summary Assign test files to presentation team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to shared ux team Contributes to: elastic#192979
## Summary Assign test files to security solution team Contributes to: elastic#192979 --------- Co-authored-by: Elastic Machine <[email protected]>
## Summary Assign test files to obs team Contributes to: elastic#192979
…thout an owner (elastic#201979) ## Summary Add quick check to prevent test files from being added without an owner Resolves: elastic#192979 Co-authored-by: Elastic Machine <[email protected]> Co-authored-by: Robert Oskamp <[email protected]>
Summary
Enable another check on PR CI, such that if a PR adds a test file that does not have an owner, the author is notified.
In support of: https://docs.google.com/document/d/1yYg-9qJGpPrGpiN2c0ZhDaBoFy9uBBbWp_z0x0Hw6Mw/edit#heading=h.t65bsv62ker1
Goals - Better Reporting (team based, better insights, how many tests, perf, etc)
FYI
This is a best effort option to find the correct code owners.
It may not be perfect at the outset.
Iteration 1 - Everything should have an owner
Iteration 2 - Prevent new files from coming into main, without an owner (
test
&&x-pack/test
&&x-pack/test_serverless
Enable check and files without owners count === 0
The text was updated successfully, but these errors were encountered: