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

[CI/CD] Stale issues workflow pinging invalid components #29744

Closed
crobert-1 opened this issue Dec 11, 2023 · 3 comments · Fixed by #29746
Closed

[CI/CD] Stale issues workflow pinging invalid components #29744

crobert-1 opened this issue Dec 11, 2023 · 3 comments · Fixed by #29746
Assignees
Labels
bug Something isn't working chore ci-cd CI, CD, testing, build issues

Comments

@crobert-1
Copy link
Member

Component(s)

No response

Describe the issue you're reporting

Description

The Mark issue as stale workflow has a bug in its logic when pinging code owners based on labels.

Here's the problem code:

LABELS=$(gh issue view "${ISSUE}" --json labels --jq '.labels.[].name')
for LABEL in ${LABELS}; do
COMPONENT="${LABEL}"

The way this is written is labels are broken up into words, not kept together as a whole.

Example:

crobert$ ~/dev/opentelemetry-collector-contrib $ LABELS=$(gh issue view 27577 --json labels --jq '.labels.[].name')
crobert$ ~/dev/opentelemetry-collector-contrib $ for LABEL in ${LABELS}; do
> echo $LABEL
> done
bug
help
wanted
good
first
issue
Stale
flaky
test
receiver/sqlquery
receiver/oracledb

The script in question will then check for valid code owners for each "label" treating these split labels as possible components. The label flaky test is split into flaky and test, and then the get-codeowners.sh script is called on each word, flaky, and test. Before #29572 the get-codeowners script would search for test, find multiple matches, then search test/test. This search would fail as there are no components named test/test. However, after #29572, get-codeowners will search test/ and return the first match as if it's a valid component. Currently, the first match is internal/k8stest, which will ping me as I'm the only code owner there.

Examples of this bug:
#24773 (comment)
#27577 (comment)

Possible solutions

  1. Add check in get-codeowners.sh to only check for codeowners if the given COMPONENT is a valid component as defined by the get-components.sh script. We should return empty code owners here and not exit unsuccessfully, as this would match current behavior of the the script. Also, I think this is a good idea as get-components.sh is what's used to generate labels for this repository.
  2. get-codeowners.sh could match from the beginning of lines, so the given COMPONENT has to match a line's start. This is valid as well because the get-components.sh script is matching the first column of the component rows, shown here.
  3. get-codeowners.sh could match entire words (-x option in grep), not just text within a line.
  4. Modify Mark issue as stale script to only ping on complete labels, not split labels.

I think a combination of 1, 2, and 4 would be best here. I don't see a time where a label should have multiple words corresponding to multiple components, so I think 4 makes sense. 1 and 2 are mostly validating functionality of the get-codeowners.sh script to make sure it properly handles invalid input. 3 may just be complicated or unclear.

@crobert-1 crobert-1 added bug Something isn't working ci-cd CI, CD, testing, build issues chore needs triage New item requiring triage labels Dec 11, 2023
@crobert-1
Copy link
Member Author

Ping @evan-bradley as I know you've done a lot of the work here.

@crobert-1
Copy link
Member Author

I'm going to remove needs triage as the main contributor to this logic is reviewing the PR.

@crobert-1 crobert-1 removed the needs triage New item requiring triage label Dec 12, 2023
@crobert-1 crobert-1 self-assigned this Dec 12, 2023
@crobert-1
Copy link
Member Author

The submitted PR implements solution 2 with a slight modification: The component match ensures whitespace after the component name in addition to being at the beginning of the line, so it must match the entire component name, not just part of it.

evan-bradley added a commit that referenced this issue Dec 12, 2023
**Description:** <Describe what has changed.>
<!--Ex. Fixing a bug - Describe the bug and how this fixes the issue.
Ex. Adding a feature - Explain what this achieves.-->
There's a combination of bugs currently existing in `get-codeowners.sh`
and the `Mark issues as stale` workflow. This implements proposed
solution 2 of #29744
- Match component against beginning of line in code owners.

**Issues**
Resolves #29744

**Testing**
Basic spot checking script:
```
COMPONENTS=("azure" "processor/resourcedetection" "processor/resourcedetectionprocessor" "test" "testbed" "extension/observer" "internal/k8stest" "extension/encoding" "flaky" "pkg/translator" "tools" )

for EXAMPLE_COMPONENT in ${COMPONENTS[@]}; do
    OWNERS=$(COMPONENT=${EXAMPLE_COMPONENT} bash ".github/workflows/scripts/get-codeowners.sh")
    echo $EXAMPLE_COMPONENT: $OWNERS
done
```
Output:
```
azure:
processor/resourcedetection: @Aneurysm9 @dashpole
processor/resourcedetectionprocessor: @Aneurysm9 @dashpole
test:
testbed: @open-telemetry/collector-approvers
extension/observer: @dmitryax @rmfitzpatrick
internal/k8stest: @crobert-1
extension/encoding: @atoulme @dao-jun @dmitryax @MovieStoreGuy @VihasMakwana
flaky:
pkg/translator:
tools:
```
Reference
[CODEOWNERS](https://github.com/open-telemetry/opentelemetry-collector-contrib/blob/main/.github/CODEOWNERS)
as source of truth.

---------

Co-authored-by: Evan Bradley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working chore ci-cd CI, CD, testing, build issues
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant