Handle branch names containing hyphen separators #450

tspencer244 · 2023-10-12T05:42:37Z

This fixes an issue for me where I was not receiving anything for "alert-state" on Dependabot pull requests despite them being associated with a Dependabot alert.

I believe the issue is caused from how the directory name is derived from the git branch name, when a hyphen is used for the branch separator.

I think the Dependabot alert processing code appears to be looking for a match between the derived-from-branch package manifest file path and actual path on the Dependabot alert, so it never matches due to incorrectly derived base directory.

Before my changes, a branch name of dependabot-npm_and_yarn-nested-nested-fsevents-1.2.13 would say the directory is /fsevents.

My assumptions:

If a manifest file exists at /nested/nested/ecosystem.json, the branch name that uses hyphen separators would be dependabot-ecosystem-nested-nested-dependency-1.2.3

he-wowers

makes sense to me

Nishnha · 2023-11-01T04:53:43Z

src/dependabot/update_metadata.ts

+        // When a branch delimiter of "-" is used, we need to +1 to end of slice because there is always a hyphen
+        // between dependency name and version at the end of the branch name, regardless of configured branch separator.
+        // e.g. "fsevents-1.2.13".
+        const baseSliceEnd = delim === '-' ? 2 : 1
+        const dirname = `/${chunks.slice(2, -1 * (baseSliceEnd + (dependency['dependency-name'].match(/\//g) || []).length)).join('/') || ''}`


The test cases LGTM and this PR makes sense, but I think we should refactor dirname since baseSliceEnd adds some more complexity.

Suggested change

// When a branch delimiter of "-" is used, we need to +1 to end of slice because there is always a hyphen

// between dependency name and version at the end of the branch name, regardless of configured branch separator.

// e.g. "fsevents-1.2.13".

const baseSliceEnd = delim === '-' ? 2 : 1

const dirname = `/${chunks.slice(2, -1 * (baseSliceEnd + (dependency['dependency-name'].match(/\//g) || []).length)).join('/') || ''}`

// When a branch delimiter of "-" is used, we need to adjust for the hyphen

// between the dependency name and version at the end of the branch name

// e.g. "fsevents-1.2.13".

const adjustForDelim = delim === '-' ? 2 : 1

const numDirectorySegments = (dependency['dependency-name'].match(/\//g) || []).length + adjustForDelim

const dirname = `/${chunks.slice(2, -1 * numDirectorySegments).join('/') || ''}`

I agree that calculating the directory name needs refactoring - will try to see if I can make it better

Alright I have tried to refactor it into something that is hopefully easier to follow - open to feedback

Nishnha · 2023-11-01T04:54:35Z

src/dependabot/update_metadata.ts

@@ -64,7 +68,7 @@ export async function parse (commitMessage: string, body: string, branchName: st
          newVersion: nextVersion,
          compatScore: await scoreFn(dependency['dependency-name'], lastVersion, nextVersion, chunks[1]),
          maintainerChanges: newMaintainer,
-          dependencyGroup: dependencyGroup,
+          dependencyGroup,


I think we have a weird linter rule that will tell you to do this? Let's ignore it for this PR

Suggested change

dependencyGroup,

dependencyGroup: dependencyGroup,

I will undo this change then

I know I had asked you to change this, but I was wrong 😭
You had it right the first time. Could you change it back and remove the eslint exception?

This ESLint rule has been fixed now

tspencer244 · 2023-11-02T09:27:09Z

src/dependabot/update_metadata.test.ts

@@ -106,7 +106,7 @@ test('it supports multiple dependencies within a single fragment', async () => {
    return Promise.resolve(0)
  }

-  const updatedDependencies = await updateMetadata.parse(commitMessage, body, 'dependabot/nuget/api/main/coffee-rails', 'main', getAlert, getScore)
+  const updatedDependencies = await updateMetadata.parse(commitMessage, body, 'dependabot/nuget/api/main/coffee-rails/and/coffeescript', 'main', getAlert, getScore)


I think the branch name for this test was incorrect to begin with?

tspencer244 · 2023-11-02T09:28:23Z

src/dry-run.ts

@@ -93,4 +93,5 @@ require('yargs')(hideBin(process.argv))
  })
  .demandCommand(1)
  .help()
+  .strict()


Adding this option makes the dry run script show an error if you forget to give the "check" argument

Nishnha · 2023-11-09T03:59:25Z

src/dependabot/update_metadata.ts

+    sliceEnd -= 1
+  }
+
+  // If there is more than 1 dependency name being updated, we assume 1 piece of the branch name will be "and".


good catch!

Nishnha · 2023-11-09T04:04:12Z

This LGTM. Before I merge could you undo the eslint exception I commented about in #450 (comment) and could you rebase this branch? It is currently out of date.

aamagda · 2024-02-27T18:15:40Z

Hi! Do you have a plan to merge this changes? Issue still actual

tspencer244 · 2024-04-24T03:50:29Z

This LGTM. Before I merge could you undo the eslint exception I commented about in #450 (comment) and could you rebase this branch? It is currently out of date.

I have undone the ESLint exception as well as rebased the pull request now

tspencer244 · 2024-04-24T03:52:07Z

Hi! Do you have a plan to merge this changes? Issue still actual

Sorry about this, I failed to notice that this pull request had been approved and was waiting for me to fix up some remaining issues.

I have fixed the remaining issues so hopefully there will be no further delay in getting this merged now.

Nishnha

Reapproving

[![Mend Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com) This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [dependabot/fetch-metadata](https://togithub.com/dependabot/fetch-metadata) | action | minor | `v2.0.0` -> `v2.1.0` | --- ### Release Notes <details> <summary>dependabot/fetch-metadata (dependabot/fetch-metadata)</summary> ### [`v2.1.0`](https://togithub.com/dependabot/fetch-metadata/releases/tag/v2.1.0) [Compare Source](https://togithub.com/dependabot/fetch-metadata/compare/v2.0.0...v2.1.0) #### What's Changed - Relax `engine-strict=true` by [@jeffwidman](https://togithub.com/jeffwidman) in [https://github.com/dependabot/fetch-metadata/pull/510](https://togithub.com/dependabot/fetch-metadata/pull/510) - Handle branch names containing hyphen separators by [@tspencer244](https://togithub.com/tspencer244) in [https://github.com/dependabot/fetch-metadata/pull/450](https://togithub.com/dependabot/fetch-metadata/pull/450) - Switch to monthly release cadence by [@jeffwidman](https://togithub.com/jeffwidman) in [https://github.com/dependabot/fetch-metadata/pull/509](https://togithub.com/dependabot/fetch-metadata/pull/509) - v2.1.0 by [@fetch-metadata-action-automation](https://togithub.com/fetch-metadata-action-automation) in [https://github.com/dependabot/fetch-metadata/pull/518](https://togithub.com/dependabot/fetch-metadata/pull/518) #### New Contributors - [@tspencer244](https://togithub.com/tspencer244) made their first contribution in [https://github.com/dependabot/fetch-metadata/pull/450](https://togithub.com/dependabot/fetch-metadata/pull/450) **Full Changelog**: dependabot/fetch-metadata@v2.0.0...v2.1.0 </details> --- ### Configuration 📅 **Schedule**: Branch creation - "after 11pm every weekday,before 7am every weekday,every weekend" in timezone Europe/Brussels, Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Mend Renovate](https://www.mend.io/free-developer-tools/renovate/). View repository job log [here](https://developer.mend.io/github/kairos-io/provider-kairos).  Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com>

tspencer244 requested a review from a team as a code owner October 12, 2023 05:42

tspencer244 marked this pull request as draft October 12, 2023 05:44

tspencer244 force-pushed the handle-branches-with-hyphens branch 2 times, most recently from cee26fc to 609e0ae Compare October 12, 2023 05:48

tspencer244 marked this pull request as ready for review October 12, 2023 05:48

he-wowers approved these changes Oct 12, 2023

View reviewed changes

Nishnha reviewed Nov 1, 2023

View reviewed changes

tspencer244 commented Nov 2, 2023

View reviewed changes

tspencer244 requested a review from Nishnha November 2, 2023 09:30

Nishnha reviewed Nov 9, 2023

View reviewed changes

Nishnha approved these changes Nov 9, 2023

View reviewed changes

Iqwanzakaria approved these changes Dec 20, 2023

View reviewed changes

tspencer244 force-pushed the handle-branches-with-hyphens branch from 9514599 to 0626347 Compare April 24, 2024 03:41

Handle branch names containing hyphen separators

a44a9df

tspencer244 force-pushed the handle-branches-with-hyphens branch from b4be41a to a44a9df Compare April 24, 2024 03:57

Nishnha approved these changes Apr 24, 2024

View reviewed changes

Nishnha merged commit 5c7b450 into dependabot:main Apr 24, 2024
6 checks passed

jeffwidman mentioned this pull request Apr 24, 2024

Directory name is not properly extracted from branch name when using - separator. #493

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle branch names containing hyphen separators #450

Handle branch names containing hyphen separators #450

tspencer244 commented Oct 12, 2023

he-wowers left a comment

Nishnha Nov 1, 2023

tspencer244 Nov 1, 2023

tspencer244 Nov 2, 2023

Nishnha Nov 1, 2023

tspencer244 Nov 1, 2023

Nishnha Nov 9, 2023

tspencer244 Apr 24, 2024

tspencer244 Nov 2, 2023

tspencer244 Nov 2, 2023

Nishnha Nov 9, 2023

Nishnha commented Nov 9, 2023

aamagda commented Feb 27, 2024

tspencer244 commented Apr 24, 2024

tspencer244 commented Apr 24, 2024

Nishnha left a comment

Handle branch names containing hyphen separators #450

Handle branch names containing hyphen separators #450

Conversation

tspencer244 commented Oct 12, 2023

he-wowers left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Nishnha commented Nov 9, 2023

aamagda commented Feb 27, 2024

tspencer244 commented Apr 24, 2024

tspencer244 commented Apr 24, 2024

Nishnha left a comment

Choose a reason for hiding this comment