-
Notifications
You must be signed in to change notification settings - Fork 2.5k
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
[chore][CI/CD] Fix github action for pinging code owners on some components #29572
[chore][CI/CD] Fix github action for pinging code owners on some components #29572
Conversation
Check another component name where you just append the name with a backslash.
@@ -9,6 +9,10 @@ | |||
|
|||
set -euo pipefail | |||
|
|||
get_codeowners() { | |||
echo "$((grep -m 1 "${1}" .github/CODEOWNERS || true) | sed 's/ */ /g' | cut -f3- -d ' ')" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is the exact same logic as before, just using the argument passed to the function instead of a global variable.
|
||
OWNERS=$( (grep -m 1 "${COMPONENT}" .github/CODEOWNERS || true) | sed 's/ */ /g' | cut -f3- -d ' ' ) | ||
if [[ -z "${OWNERS:-}" ]]; then |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Existing code structure should be the same as before, just a single condition is added. Originally there was a single statement setting OWNERS
because COMPONENT
was modified to have the COMPONENT_TYPE
appended to it.
So even though there's an if/else condition here now, it's the same logic, just an extra condition for the /
appended instead of COMPONENT_TYPE
.
If we want to really ensure this worked, we can add labels to this PR to make sure code owners are pinged. It would be noisy for them, but probably better than merging if I somehow broke something. Otherwise I can test on an issue this is broken for after its merged. |
Let's try it with one component and see if it works. |
It doesn't I guess since it applies the script from main, so we need to merge this to see if it works. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really appreciate you taking a look at this.
Description:
Existing bug:
This resolves a bug in pinging code owners that was hit when a base component doesn't end with its type, but has multiple sub-components.
For example, when adding the label
extension/encoding
to issues code owners weren't being pinged. This is because there are multiple sub-components (e.g.extension/encoding/jaegarencodingextension
,extension/encoding/zipkinencodingextension
, etc), andextension/encoding
doesn't end with its own typeextension
.Solution:
The solution to this bug is to also check if the original component can be found if a single
/
is appended to it. This finds the component in a determinate way and allows code owners to be pinged properly.Link to tracking Issue:
Resolves #29571
Testing:
Previously,
extension/observer
andextension/encoding
returned nothing, which was the bug this resolves.