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

Fix dependency tag filtering to use exact prefix matching #10799

Merged
merged 3 commits into from
Oct 17, 2024

Conversation

robaiken
Copy link
Contributor

What are you trying to accomplish?

I am fixing an issue with dependency tag filtering in our release notes finder process. Currently, the system incorrectly identifies releases when unrelated tags contain the package name. I'm changing the tag filtering method from included? to start_with? to ensure more accurate matching.

This change prevents false positives in release identification, ensuring that our automated dependency update process works correctly and doesn't miss important updates or include irrelevant ones.

Anything you want to highlight for special attention from reviewers?

The key change is in the tag filtering logic:

# Old approach
.select { |r| r.tag_name.included?(dep_prefix) }

# New approach
.select { |r| r.tag_name.start_with?(dep_prefix) }

I chose this approach because it provides a more precise match for dependency tags. While using included? was too broad and led to false positives, start_with? ensures we only match tags that explicitly start with the dependency name.

Please pay special attention to the updated test cases, particularly the new fixture file releases_with_mixed_tag_formats.json, which aims to cover the tag naming scenarios.

How will you know you've accomplished your goal?

  1. The automated tests pass, particularly the new/updated tests that check for correct tag retention.
  2. In real-world scenarios, we see a reduction in incorrectly identified releases.
  3. The system correctly identifies releases even when unrelated tags contain the package name elsewhere in the tag.

Checklist

  • I have run the complete test suite to ensure all tests and linters pass.
  • I have thoroughly tested my code changes to ensure they work as expected, including adding additional tests for new functionality.
  • I have written clear and descriptive commit messages.
  • I have provided a detailed description of the changes in the pull request, including the problem it addresses, how it fixes the problem, and any relevant details about the implementation.
  • I have ensured that the code is well-documented and easy to understand.

@robaiken robaiken requested a review from a team as a code owner October 16, 2024 14:40
Copy link
Contributor

@markhallen markhallen left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Member

@jakecoffman jakecoffman left a comment

Choose a reason for hiding this comment

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

Any reason we wouldn't want to use an exact match?

@robaiken robaiken enabled auto-merge (squash) October 17, 2024 09:35
@robaiken
Copy link
Contributor Author

@jakecoffman It looks like the original intention of this code was to check for a prefix, rather than check the whole string. Although all examples I've seen checking the exact text would work, checking it as a prefix seems to be the safer choice.

@robaiken robaiken merged commit a4b9936 into main Oct 17, 2024
123 checks passed
@robaiken robaiken deleted the robaiken/check-tag-prefix branch October 17, 2024 09:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants