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

Switch depName/packageName logic #16012

Open
rarkins opened this issue Jun 11, 2022 · 22 comments
Open

Switch depName/packageName logic #16012

rarkins opened this issue Jun 11, 2022 · 22 comments
Labels
core:package-rules Relating to package-rules e.g. matchers priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)

Comments

@rarkins
Copy link
Collaborator

rarkins commented Jun 11, 2022

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 rarkins added type:feature Feature (new functionality) priority-2-high Bugs impacting wide number of users or very important features status:requirements Full requirements are not yet known, so implementation should not be started labels Jun 11, 2022
@viceice
Copy link
Member

viceice commented Jun 11, 2022

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

@PhilipAbed
Copy link
Collaborator

PhilipAbed commented Jun 13, 2022

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?

@rarkins
Copy link
Collaborator Author

rarkins commented Jun 13, 2022

  • 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

@viceice
Copy link
Member

viceice commented Jun 13, 2022

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

@PhilipAbed
Copy link
Collaborator

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

@rarkins
Copy link
Collaborator Author

rarkins commented Jun 14, 2022

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

@MaronHatoum
Copy link
Contributor

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

config.packageName getting the value of config.depName:

const packageName = config.packageName ?? config.depName;

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

definition:

const matchPackageNames = packageRule.matchPackageNames ?? [];
let matchPackagePatterns = packageRule.matchPackagePatterns ?? [];

usage:

let isMatch = matchPackageNames.includes(depName);

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.

@rarkins
Copy link
Collaborator Author

rarkins commented Aug 1, 2022

  • 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

@dalbani
Copy link

dalbani commented Jul 18, 2023

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?

@rarkins
Copy link
Collaborator Author

rarkins commented Jul 24, 2023

@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)

@morremeyer
Copy link
Contributor

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?

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 13, 2023

@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)

@morremeyer
Copy link
Contributor

@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.

@rarkins
Copy link
Collaborator Author

rarkins commented Sep 13, 2023

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 rarkins added status:ready and removed impact:large status:requirements Full requirements are not yet known, so implementation should not be started labels Sep 26, 2023
@rarkins
Copy link
Collaborator Author

rarkins commented Oct 11, 2023

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 rarkins added priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others and removed priority-2-high Bugs impacting wide number of users or very important features labels Oct 12, 2023
@not7cd

This comment was marked as resolved.

@rarkins

This comment was marked as resolved.

@morremeyer

This comment was marked as resolved.

@rarkins
Copy link
Collaborator Author

rarkins commented Apr 22, 2024

Proposed typing change to lib/manager/types:

image

@viceice
Copy link
Member

viceice commented Apr 22, 2024

Needs more context but looks ok to me

@wwuck
Copy link
Contributor

wwuck commented May 3, 2024

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

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

@rarkins
Copy link
Collaborator Author

rarkins commented May 5, 2024

See #28834

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core:package-rules Relating to package-rules e.g. matchers priority-3-medium Default priority, "should be done" but isn't prioritised ahead of others type:feature Feature (new functionality)
Projects
None yet
Development

No branches or pull requests

9 participants