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.

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][Ownership] Fixup test_serverless entries #195609

Open
wants to merge 23 commits into
base: main
Choose a base branch
from

Conversation

wayneseymour
Copy link
Member

Summary

Changing lines from security-solution to other team, per request.

Contributes to: #194815

Addresses: https://github.com/elastic/kibana/pull/194819/files/d4eb7a8b397e15357790ba32a0266e724d2b775b#r1792990254

@wayneseymour wayneseymour added release_note:skip Skip the PR/issue when compiling release notes backport:skip This commit does not require backporting FTR labels Oct 9, 2024
@wayneseymour wayneseymour self-assigned this Oct 9, 2024
@wayneseymour wayneseymour requested a review from a team October 9, 2024 14:11
@azasypkin azasypkin removed the request for review from a team October 9, 2024 14:49
@wayneseymour
Copy link
Member Author

@azasypkin
Hello again, as the changes are for @elastic/kibana-security and you've removed this group, any idea whom should be a reviewer?

Further, should they not be assigned to kibana security?

@azasypkin
Copy link
Member

Further, should they not be assigned to kibana security?

Yeah, none of the test paths mentioned in this PR should be owned by the AppEx Platform Security team. Since these are under security sub-path I expect Security Solution to help with finding the owners, I'd suggest asking in #security-unified-app.

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour wayneseymour requested a review from a team October 18, 2024 12:01
@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@wayneseymour wayneseymour requested review from joemcelroy and removed request for joemcelroy October 21, 2024 09:48
@wayneseymour wayneseymour requested review from a team October 25, 2024 08:39
@@ -1297,6 +1298,7 @@ x-pack/test_serverless/**/test_suites/observability/ai_assistant @elastic/obs-ai
#CC# /src/plugins/kibana_react/public/code_editor/ @elastic/kibana-presentation

# Machine Learning
/x-pack/test_serverless/functional/test_suites/security/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.

Is this needed? We already have /x-pack/test_serverless/**/test_suites/**/ml/ @elastic/ml-ui below.

Copy link
Member Author

@wayneseymour wayneseymour Oct 25, 2024

Choose a reason for hiding this comment

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

I ran the following on the main branch:

$ node scripts/get_owners_for_file.js --file x-pack/test_serverless/functional/test_suites/security/ml
ERROR Ownership of file [x-pack/test_serverless/functional/test_suites/security/ml] is UNKNOWN

Looks like we need an assignment Pete.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don't know why the wildcard ** aren't working. There are a lot of other uses of the wildcard syntax in this file. Do any of them work as intended?

If the ** isn't working, then you'd also have to add entries for

/x-pack/test_serverless/functional/test_suites/observability
/x-pack/test_serverless/functional/test_suites/search

Copy link
Member Author

Choose a reason for hiding this comment

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

Looks like you correct:

node scripts/get_owners_for_file.js --file x-pack/test_serverless/functional/test_suites/observability
ERROR Ownership of file [x-pack/test_serverless/functional/test_suites/observability] is UNKNOWN

As I'm responsible for assigning all of them, and ensuring this script returns zero missing owners as apart of this effort, this will most definitely be followed up in a PR soon.

It'll all be fixed up sooner than later.

Copy link
Member Author

Choose a reason for hiding this comment

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

Furthermore, a follow-on project is coming to co-locate tests with what they test (plugins, packages, etc).
So the codeowners file will get an additional cleanup then as well.

Copy link
Member Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

With the investigations done as part of #197805, we know that /x-pack/test_serverless/functional/test_suites/security/ml is actually covered by the /x-pack/test_serverless/**/test_suites/**/ml/ @elastic/ml-ui entry and the updated check script will reflect that once that PR is merged. So this additional entry can be removed.

Copy link
Member Author

Choose a reason for hiding this comment

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

@wayneseymour
Copy link
Member Author

@neptunian fe462c0

@wayneseymour wayneseymour marked this pull request as draft October 25, 2024 14:11
@wayneseymour wayneseymour requested a review from a team October 25, 2024 14:52
@wayneseymour wayneseymour requested review from a team and removed request for a team October 28, 2024 15:32
@wayneseymour wayneseymour marked this pull request as ready for review October 28, 2024 15:33
Copy link
Member

@kpollich kpollich left a comment

Choose a reason for hiding this comment

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

Updated Fleet ownership LGTM

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Metrics [docs]

✅ unchanged

History

cc @wayneseymour

@wayneseymour
Copy link
Member Author

@elasticmachine merge upstream

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport:skip This commit does not require backporting FTR release_note:skip Skip the PR/issue when compiling release notes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants