renovate icon indicating copy to clipboard operation
renovate copied to clipboard

Switch depName/packageName logic

Open rarkins opened this issue 2 years ago • 22 comments

What would you like Renovate to be able to do?

Instead of defaulting to depName, and allowing packageName as an optional override, we should instead default to extracting packageName, and allowing depName optionally.

If you have any ideas on how this should be implemented, please tell us here.

We would need to make changes to each manager's extract function, which will touch most managers but not be complex.

There may be a small change to datasource index logic because now it does not need the depName->packageName fallback.

But then there are other areas of the code which assume depName is always present.

One in particular is packageRules. Today matchPackage* actually functions on the depName field and not the packageName. We might need to do something like:

  • Try matching against packageName first
  • If a depName is present, try matching against it second

Or maybe instead we could:

  • add new matchDepNames and matchDepNamePatterns fields
  • Do like above, but WARN if the match is on depName for one major release
  • Deprecate the depName matching fallback in a later release

Ultimately this issue is going to require some exploration and testing - it's hard to predict everywhere it might cause changes

Is this a feature you are interested in implementing yourself?

Maybe

rarkins avatar Jun 11 '22 05:06 rarkins

sounds good, i think we can do it in multiple steps.

viceice avatar Jun 11 '22 10:06 viceice

let's clear some things up:

  • We will need to fix every extract function for every manager? can we do it 1 manager at a time in small tasks or 3-4 managers at a time depends on how long it takes for one? what about templates?
  • For our information: What's the difference between packageName and depName?
  • and what about current users.. we wont have backward compatiblity causing this to be BREAKING changes?

PhilipAbed avatar Jun 13 '22 10:06 PhilipAbed

  • We will need to fix every extract function for every manager? can we do it 1 manager at a time in small tasks or 3-4 managers at a time depends on how long it takes for one? what about templates?

I think it's easiest to do them all at once.

We will need to think about the repercussions for regexManagers.

  • For our information: What's the difference between packageName and depName?

By default, only depName is needed and that's what's used to look up packages. If packageName is also specified then it means depName is essentially "display name" while packageName is "lookup name".

  • and what about current users.. we wont have backward compatiblity causing this to be BREAKING changes?

We would aim for it not to be breaking

rarkins avatar Jun 13 '22 19:06 rarkins

So regex manager should set packageName=depName if only depName is matched

viceice avatar Jun 13 '22 20:06 viceice

that's what i was thinking about current users, they will need to stop using depName in their regex managers, and start using packageName... it would be breaking i believe

PhilipAbed avatar Jun 14 '22 09:06 PhilipAbed

That's not what @viceice meant. He meant:

  • if both depName and packageName are in the regex manager result then keep it as-is
  • if only depName is found then rename it to packageName once extraction/template compilation is done

rarkins avatar Jun 14 '22 11:06 rarkins

Today config.packageName is optional and config.depName is Required.

config.packageName getting the value of config.depName:

https://github.com/renovatebot/renovate/blob/e4dbd4ad491f656aaa7f9da9d4dae2c18622a1e6/lib/modules/datasource/index.ts#L344

in packageRules we have matchPackagePatterns and matchPackageNames these matchers are related to packgeName.

definition: https://github.com/renovatebot/renovate/blob/e4dbd4ad491f656aaa7f9da9d4dae2c18622a1e6/lib/util/package-rules.ts#L41-L42

usage:

https://github.com/renovatebot/renovate/blob/e4dbd4ad491f656aaa7f9da9d4dae2c18622a1e6/lib/util/package-rules.ts#L160

Instead of trying matching against depName, we should change it to be against packageName.

then we should add 2 new matchers to the depName (matchDepPatterns , matchDepNames).

need to check if depName matched matchDepNames and if packageName matched matchPackageNames

  • if yes keep the values as is
  • else
  • if just depName matched so set packageName=depName.

MaronHatoum avatar Jul 31 '22 11:07 MaronHatoum

  • Switch all managers to extract packageName by default instead of depName
  • Add new match fields to packageRules
  • Add logic to match matchPackageX against packageName first, then try depName. If packageName doesn't match but depName does, we should warn the user
  • Massage depName to be packageName if it's empty

I don't think there's any reason to set packageName=depName as you mentioned

rarkins avatar Aug 01 '22 04:08 rarkins

Landing here from a Google search while trying to figure why I suddenly(?) get this warning on a project in a local on-prem Gitlab instance running with whitesource/renovate:5.0.0:

image

With a Renovate Bot configuration like:

{
  ...
  "regexManagers": [
    {
      "description": "*_VERSION variables in Dockerfiles",
      "fileMatch": ["(^|\\/|\\.)Dockerfile$", "(^|/)Dockerfile\\.[^/]*$"],
      "matchStrings": [
        "# renovate: datasource=(?<datasource>[a-z-]+?) depName=(?<depName>.+?)(?: (?:packageName|lookupName)=(?<packageName>.+?))?(?: versioning=(?<versioning>[a-z-]+?))?\\s(?:ENV|ARG) .+?_VERSION=(?<currentValue>.+?)\\s"
      ],
      "versioningTemplate": "{{#if versioning}}{{versioning}}{{else}}semver{{/if}}"
    }
  ]
}

In combination with a Dockerfile like:

...
# renovate: datasource=maven depName=com.microsoft.playwright:playwright
ENV PLAYWRIGHT_VERSION=1.36.0
...

Strangely enough, this warning doesn't pop up on another Git project with an almost identical configuration 🧐

So is this related to the current issue?

dalbani avatar Jul 18 '23 10:07 dalbani

@dalbani better if you start a fresh discussion about this, ideally with a reproduction repo if it's possible.

In short: matchPackageNames used to match against depName, not packageName. We're trying to transition without breaking people, so now we support matchDepNames too and warn someone if there's code which should change from matchPackageNames to matchDepNames. It's possible that you're getting a warning due to some built-in presets which include packageRules (i.e. not directly from your repo)

rarkins avatar Jul 24 '23 05:07 rarkins

Do I see it correctly that there are still some presets using matchPackageNames instead of matchDepNames that should be updated?

Searching with https://github.com/search?q=repo%3Arenovatebot%2Frenovate%20path%3A%2F%5Elib%5C%2Fconfig%5C%2Fpresets%5C%2F%2F%20matchPackageNames&type=code, I see quite a few presets.

We are getting the Use matchDepNames instead of matchPackageNames warning e.g. for node.

If I'm readying this right, I'm happy to open a PR that replaces all matchPackageNames in the presets with matchDepNames if that's the correct approach - or do we need to keep some matchPackageNames occurrences?

morremeyer avatar Sep 13 '23 12:09 morremeyer

@morremeyer yes I think you have it right. I haven't yet done a search through our presets to see if any should swap from matchPackageNames to matchDepNames, so it would be appreciated if you can do that and raise a PR!

BTW the only time we need matchDepNames is when depName and packageName are different, and node is one of those because sometimes we massage them (e.g. from amd64/node to node)

rarkins avatar Sep 13 '23 12:09 rarkins

@rarkins But we do still need either of the two for a packageRule and as far as I understand this issue, the move is towards using depName, is that correct?

To be honest, every time I read up on the difference between the two, I forget it very quickly afterwards and there's no explanatory documentation about the differences and intricacies of the two, so I need to get myself up to speed every time 😁

Might be worth to add that somewhere.

morremeyer avatar Sep 13 '23 13:09 morremeyer

The way it works today is: always have depName, use packageName optionally if the "lookup name" is different. Warn if a matchPackageNames matches depName but not packageName.

The way it will work in future is: always have packageName, use depName optionally as a "pretty name"/"short name". Hence why we warn today, because in future we don't fall back to checking depName for matchPackageNames.

packageName is probably still right for the actual package name we look up (note: used to be called lookupName). We can reconsider depName after the refactor

rarkins avatar Sep 13 '23 13:09 rarkins

I would like to achieve this by having more strict typings for manager extractions.

Dependencies need to have datasource and packageName at minimum, while depName and registryUrls are optional.

We sometimes also extract incomplete and skipped dependencies, in which case skipReason is set. I don't see the benefit of extracted dependencies with only skipReason set and think we should include a currentValue at least too. The benefit of including skipped dependencies is that they're shown in the logs instead of invisible

rarkins avatar Oct 11 '23 18:10 rarkins

One in particular is packageRules. Today matchPackage* actually functions on the depName field and not the packageName.

Is this statement still true?

not7cd avatar Mar 06 '24 12:03 not7cd

Today matchPackage* tries matching packageName first, then falls back to depName, and logs an info message if depName matches when packageName did not: https://github.com/renovatebot/renovate/blob/14272f07a26fe13b11aa207fc7aba108bc764277/lib/util/package-rules/package-names.ts#L23-L27

We should raise this to a warning, and then after that remove the fallback.

rarkins avatar Mar 06 '24 14:03 rarkins

Quick update regarding my comment https://github.com/renovatebot/renovate/issues/16012#issuecomment-1717497623 if anyone is looking for that: This change has been implemented by someone else in the meantime. Thank you!

morremeyer avatar Mar 19 '24 11:03 morremeyer

Proposed typing change to lib/manager/types:

image

rarkins avatar Apr 22 '24 05:04 rarkins

Needs more context but looks ok to me

viceice avatar Apr 22 '24 06:04 viceice

Does this documentation need to be changed from depName to packageName?

https://docs.renovatebot.com/modules/manager/regex/#advanced-capture

wwuck avatar May 03 '24 04:05 wwuck

See https://github.com/renovatebot/renovate/pull/28834

rarkins avatar May 05 '24 06:05 rarkins