-
Notifications
You must be signed in to change notification settings - Fork 2.8k
[chore][ping code owners] Attempt to match component and label identically when possible #39145
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
Conversation
@@ -68,6 +70,7 @@ for COMPONENT in ${BODY_COMPONENTS}; do | |||
fi | |||
done | |||
|
|||
# TODO: This check isn't working, it always returns no related components |
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.
I don't understand, should this code be removed or is there a follow-up?
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 should be investigated in the future. From my local testing I couldn't get this check to fail, so it's more of an open question whether or not the check is useful or should be removed.
…cally when possible (open-telemetry#39145) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The logic for pinging code owners where multiple labels matched a given component returned the first in alphabetical order. This worked generally as the first alphabetical response was the minimal matching value, as shown in open-telemetry#38844. However, this breaks on the kafka receiver labels. The component labels are sorted in alphabetical order by the full component name. This means `receiver/kafkametricsreceiver` is before `receiver/kafkareceiver` in the list. When the `receiver/kafka` component is referenced, the `receiver/kafkametrics` label is returned as it's the first matching label in alphabetical order. This change is to try to match component and label identically. If there's no identical match, return the first label in alphabetical order. Existing logic has been broken out into a separate script to avoid duplicating the logic. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Unexpected behavior was found in open-telemetry#39129. `receiver/kafka` was the component, the label `receiver/kafkametrics` was added incorrectly, and the code owners were pinged for `receiver/kafka` correctly. When I removed the `receiver/kafkametrics` label and added `receiver/kafka`, the code owners were pinged for `receiver/kafkametrics` incorrectly. <!--Describe what testing was performed and which tests were added.--> #### Testing ``` # Testing issue where bug was detected $ .github/workflows/scripts/ping-codeowners-on-new-issue.sh No related components were given Adding the following labels: receiver/kafka Pinging code owners: - receiver/kafka: @pavolloffay @MovieStoreGuy @axw See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. # Testing other components $ export COMPONENT="os:windows" $ .github/workflows/scripts/get-label-from-component.sh $ export COMPONENT="receiver/hostmetrics" $ .github/workflows/scripts/get-label-from-component.sh receiver/hostmetrics $ export COMPONENT="receiver/kafka" $ .github/workflows/scripts/get-label-from-component.sh receiver/kafka $ export COMPONENT="receiver/kafkametrics" $ .github/workflows/scripts/get-label-from-component.sh receiver/kafkametrics $ export COMPONENT="extension/encoding" $ .github/workflows/scripts/get-label-from-component.sh extension/encoding $ export ISSUE=39129 $ COMPONENT="receiver/kafka" $ .github/workflows/scripts/ping-codeowners-issues.sh Pinging code owners for receiver/kafka: @pavolloffay @MovieStoreGuy @axw. See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label. $ COMPONENT="receiver/kafkametrics" $ .github/workflows/scripts/ping-codeowners-issues.sh Pinging code owners for receiver/kafkametrics: @dmitryax. See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label. ```
…cally when possible (open-telemetry#39145) <!--Ex. Fixing a bug - Describe the bug and how this fixes the issue. Ex. Adding a feature - Explain what this achieves.--> #### Description The logic for pinging code owners where multiple labels matched a given component returned the first in alphabetical order. This worked generally as the first alphabetical response was the minimal matching value, as shown in open-telemetry#38844. However, this breaks on the kafka receiver labels. The component labels are sorted in alphabetical order by the full component name. This means `receiver/kafkametricsreceiver` is before `receiver/kafkareceiver` in the list. When the `receiver/kafka` component is referenced, the `receiver/kafkametrics` label is returned as it's the first matching label in alphabetical order. This change is to try to match component and label identically. If there's no identical match, return the first label in alphabetical order. Existing logic has been broken out into a separate script to avoid duplicating the logic. <!-- Issue number (e.g. open-telemetry#1234) or full URL to issue, if applicable. --> #### Link to tracking issue Unexpected behavior was found in open-telemetry#39129. `receiver/kafka` was the component, the label `receiver/kafkametrics` was added incorrectly, and the code owners were pinged for `receiver/kafka` correctly. When I removed the `receiver/kafkametrics` label and added `receiver/kafka`, the code owners were pinged for `receiver/kafkametrics` incorrectly. <!--Describe what testing was performed and which tests were added.--> #### Testing ``` # Testing issue where bug was detected $ .github/workflows/scripts/ping-codeowners-on-new-issue.sh No related components were given Adding the following labels: receiver/kafka Pinging code owners: - receiver/kafka: @pavolloffay @MovieStoreGuy @axw See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. # Testing other components $ export COMPONENT="os:windows" $ .github/workflows/scripts/get-label-from-component.sh $ export COMPONENT="receiver/hostmetrics" $ .github/workflows/scripts/get-label-from-component.sh receiver/hostmetrics $ export COMPONENT="receiver/kafka" $ .github/workflows/scripts/get-label-from-component.sh receiver/kafka $ export COMPONENT="receiver/kafkametrics" $ .github/workflows/scripts/get-label-from-component.sh receiver/kafkametrics $ export COMPONENT="extension/encoding" $ .github/workflows/scripts/get-label-from-component.sh extension/encoding $ export ISSUE=39129 $ COMPONENT="receiver/kafka" $ .github/workflows/scripts/ping-codeowners-issues.sh Pinging code owners for receiver/kafka: @pavolloffay @MovieStoreGuy @axw. See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label. $ COMPONENT="receiver/kafkametrics" $ .github/workflows/scripts/ping-codeowners-issues.sh Pinging code owners for receiver/kafkametrics: @dmitryax. See [Adding Labels via Comments](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/CONTRIBUTING.md#adding-labels-via-comments) if you do not have permissions to add labels yourself. For example, comment '/label priority:p2 -needs-triaged' to set the priority and remove the needs-triaged label. ```
Description
The logic for pinging code owners where multiple labels matched a given component returned the first in alphabetical order. This worked generally as the first alphabetical response was the minimal matching value, as shown in #38844. However, this breaks on the kafka receiver labels. The component labels are sorted in alphabetical order by the full component name. This means
receiver/kafkametricsreceiver
is beforereceiver/kafkareceiver
in the list. When thereceiver/kafka
component is referenced, thereceiver/kafkametrics
label is returned as it's the first matching label in alphabetical order.This change is to try to match component and label identically. If there's no identical match, return the first label in alphabetical order.
Existing logic has been broken out into a separate script to avoid duplicating the logic.
Link to tracking issue
Unexpected behavior was found in #39129.
receiver/kafka
was the component, the labelreceiver/kafkametrics
was added incorrectly, and the code owners were pinged forreceiver/kafka
correctly. When I removed thereceiver/kafkametrics
label and addedreceiver/kafka
, the code owners were pinged forreceiver/kafkametrics
incorrectly.Testing