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

Jump to bottom

Remove missing files and folders from CODEOWNERS file #82060

Merged
merged 6 commits into from
Nov 10, 2020
Merged

Remove missing files and folders from CODEOWNERS file #82060

merged 6 commits into from
Nov 10, 2020

Conversation

stacey-gammon
Copy link
Contributor

Noticed a bunch of these while tracking information about missing plugin READMEs. These were determined programmatically and removed manually. Fixed a couple that were missing bc of typos.

@stacey-gammon stacey-gammon marked this pull request as ready for review October 29, 2020 18:41
@stacey-gammon stacey-gammon force-pushed the 2020-10-29-codeowners-fixes branch from a56c6de to 997aa1b Compare October 29, 2020 18:43
@stacey-gammon stacey-gammon requested a review from spalger October 29, 2020 20:27
@stacey-gammon stacey-gammon added release_note:skip Skip the PR/issue when compiling release notes v7.11 v8.0.0 labels Oct 29, 2020
stacey-gammon
stacey-gammon commented Oct 29, 2020
View reviewed changes
.github/CODEOWNERS
@@ -272,11 +208,7 @@
/x-pack/test/security_functional/ @elastic/kibana-security
/x-pack/test/spaces_api_integration/ @elastic/kibana-security
/x-pack/test/token_api_integration/ @elastic/kibana-security
#CC# /src/legacy/ui/public/capabilities @elastic/kibana-security
#CC# /x-pack/legacy/plugins/encrypted_saved_objects/ @elastic/kibana-security
#CC# /x-pack/plugins/security_solution/ @elastic/kibana-security
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FYI @elastic/siem - moved this to your team.

@timroes timroes added v7.11.0 and removed v7.11 labels Oct 30, 2020
pheyos
pheyos approved these changes Oct 30, 2020
View reviewed changes
Copy link
Member

@pheyos pheyos left a comment

Choose a reason for hiding this comment

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

ML changes LGTM

@stacey-gammon stacey-gammon force-pushed the 2020-10-29-codeowners-fixes branch from 83cd849 to 5845290 Compare October 30, 2020 13:30
spalger
spalger approved these changes Nov 2, 2020
View reviewed changes
Copy link
Contributor

@spalger spalger left a comment

Choose a reason for hiding this comment

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

LGTM, do you think the code you used to validate this could be added as a CI check that fails when PRs remove folders which are still referenced by codeowners? If you want to send me the code or just put it up in a new PR I'd be happy to wire it up to CI.

@pheyos
Copy link
Member

pheyos commented Nov 2, 2020

LGTM, do you think the code you used to validate this could be added as a CI check that fails when PRs remove folders which are still referenced by codeowners? If you want to send me the code or just put it up in a new PR I'd be happy to wire it up to CI.

@spalger This CI check would force users to update the codeowners file as part of their feature/test PR. Not sure about the preferred way here, but wouldn't it be better to modify codeowners in a separate PR as this file only exists in master and it's easy to forget that during the backport process?

@spalger
Copy link
Contributor

spalger commented Nov 2, 2020

@pheyos I think that's a reasonable approach to take, but I also sort of doubt that people are doing it that way and I think it would be reasonable/easy to extend this check to make sure that codeowners aren't being backported to non-master branches so that related changes can stay in a single PR and we can make sure folks aren't accidentally backporting changes to the codeowners config.

@pheyos
Copy link
Member

pheyos commented Nov 2, 2020

@spalger sounds good. Thanks for the explanation.

@pheyos
Copy link
Member

pheyos commented Nov 3, 2020

@stacey-gammon Since you raised this PR, we've added basic license tests for the ML and Transform apps and would like to have the new directories in the CODEOWNERS file. Happy to raise a PR for this, but in order to reduce the potential of conflicting merges, maybe you could add the following entries to the # Machine Learning section when you update your branch?

/x-pack/test/api_integration_basic/apis/ml/ @elastic/ml-ui
/x-pack/test/functional_basic/apps/ml/ @elastic/ml-ui

/x-pack/test/api_integration_basic/apis/transform/ @elastic/ml-ui
/x-pack/test/functional_basic/apps/transform/ @elastic/ml-ui

Please let me know if you prefer a separate PR for it.

@stacey-gammon
Copy link
Contributor Author

@spalger - code is here: https://github.com/stacey-gammon/issue-crawler/blob/master/src/code_crawler.ts#L60

@pheyos - will merge those changes here.

@stacey-gammon stacey-gammon force-pushed the 2020-10-29-codeowners-fixes branch from 848a097 to 0b60c9e Compare November 3, 2020 18:03
@stacey-gammon stacey-gammon merged commit 44d45ff into elastic:master Nov 10, 2020
@stacey-gammon stacey-gammon deleted the 2020-10-29-codeowners-fixes branch November 10, 2020 23:27
@stacey-gammon stacey-gammon mentioned this pull request Nov 11, 2020
Merged
watson
watson reviewed Nov 11, 2020
View reviewed changes
.github/CODEOWNERS
/x-pack/test/functional/services/transform_ui/ @elastic/ml-ui
/x-pack/test/functional/services/transform.ts @elastic/ml-ui
/x-pack/test/functional/services/transform/ @elastic/ml-ui/
x-pack/test/api_integration_basic/apis/ml/ @elastic/ml-ui
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have a leading /?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Looks like the leading / accidentally went to the end of the previous line. Already fixed in #83233.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

🤦‍♀️

@kibanamachine kibanamachine added the backport missing Added to PRs automatically when the are determined to be missing a backport. label Nov 12, 2020
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 82060 or prevent reminders by adding the backport:skip label.

3 similar comments
@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 82060 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 82060 or prevent reminders by adding the backport:skip label.

@kibanamachine
Copy link
Contributor

Friendly reminder: Looks like this PR hasn’t been backported yet.
To create backports run node scripts/backport --pr 82060 or prevent reminders by adding the backport:skip label.

@pheyos pheyos added backport:skip This commit does not require backporting and removed backport missing Added to PRs automatically when the are determined to be missing a backport. v7.11.0 labels Nov 18, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@pheyos pheyos pheyos approved these changes

@watson watson watson left review comments

@spalger spalger spalger approved these changes

Assignees
No one assigned
Labels
backport:skip This commit does not require backporting release_note:skip Skip the PR/issue when compiling release notes v8.0.0
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@stacey-gammon @pheyos @spalger @kibanamachine @watson @timroes