renovatebot / renovate

Home of the Renovate CLI: Cross-platform Dependency Automation by Mend.io
https://mend.io/renovate
GNU Affero General Public License v3.0
17.6k stars 2.32k forks source link

Subpresets no longer working since v29.1.0 #12595

Closed timmkrause closed 2 years ago

timmkrause commented 3 years ago

How are you running Renovate?

Self-hosted

If you're self-hosting Renovate, tell us what version of Renovate you run.

29.3.5 (28.26.0 still works for us)

Please select which platform you are using if self-hosting.

Azure DevOps Server

If you're self-hosting Renovate, tell us what version of the platform you run.

Azure DevOps Server 2020 Update 1

Describe the bug

We're using local presets and with 29.3.5 these stopped working for us, the shared config seems to not being applied anymore. So it happened that - because of missing groupings - we had a huge amount of PRs in front of us and our build agents were burning. :) Downgraded to 28.26.0 and it works fine again. We couldn't find anything in the logs regarding handling of presets (detection/errors) even with LOG_LEVEL=debug.

Unfortunately with the given self-hosted setup it's very hard to create a repro.

This is the setup repo/config-wise:

renovate repo

azure-pipelines.yml

npx renovate --host-rules="[{\"matchHost\":\"company.com\",\"username\":\"irrelephant\",\"password\":\"$(COMPANY_AZURE_DEVOPS_NUGET_PAT)\"}]"

config.js

module.exports = {
  platform: "azure",
  endpoint: "https://company.com/org/",
  token: process.env.system_accesstoken,  
  repositories: [
    "org/target-repo-a",
    // ...
  ],
  onboardingConfig: { "extends": ["local>project/renovate"] }
};

default.json

{
    "$schema": "https://docs.renovatebot.com/renovate-schema.json",
    // ...
    "packageRules": [
        {
            "description": "Group all COMPANY packages into single PRs to reduce noise.",
            "matchPackagePrefixes": [
                "Company"
            ],
            "groupName": "COMPANY dependencies"
        }
        // ...
    ]
}

target-repo-a repo

renovate.json

{
  "$schema": "https://docs.renovatebot.com/renovate-schema.json",
  "extends": [
    "local>project/renovate"
  ]
}

Relevant debug logs

No response

Have you created a minimal reproduction repository?

No reproduction repository

rarkins commented 3 years ago

So there's no error but you think every single preset config is not being applied?

timmkrause commented 3 years ago

Exactly. The only symptom we can observe is that the settings in default.json (especially groupings) are not being applied.

rarkins commented 3 years ago

Do you have any way to determine which release between 28.26.0 and 29.3.5 broke it for you, without causing yourself further noise/pain?

My best guess would be 29.1.0, caused by #11565, although there wasn't any specific Azure code in that.

timmkrause commented 3 years ago

Yep, thanks! I will get back with the exact version.

timmkrause commented 3 years ago

Impressing. Your best guess was 100% accurate. 29.0.1 works, 29.1.0 breaks it.

These debug messages are missing as of 29.1.0:

DEBUG: Dependency Microsoft.Extensions.Http is part of group Microsoft dependencies (repository=foo/bar)

rarkins commented 3 years ago

Are you able to disclose the actual string for "local>project/renovate"? I'm wondering if there's some special characters or something like that

timmkrause commented 3 years ago

I kept the whole pattern: local>A2B CD/A2B_Renovate

Spaces are evil?

vntw commented 2 years ago

Hey, looks like this is a general issue, not specific to Azure. Just spent a few hours debugging this since it stopped working for me too - we're using the following preset format in hosted GitLab:

gitlab>some/group/configs:presetfile/presetname(1, monday)

This accesses a presetfile.json and searches for the presetname key after loading the file.

29.0.1

This works as expected:

it('parses gitlab custom preset file with name and params', () => {
  expect(presets.parsePreset('gitlab>some/group/configs:presetfile/presetname(1, monday)')).toEqual({
    packageName: 'some/group/configs',
    params: ["1","monday"],
    presetName: 'presetfile/presetname',
    presetPath: undefined,
    presetSource: 'gitlab',
  });
});

29.0.1 with 75798754151ef4078eff9ba0a57b8f861678c718 cherry-picked (included in 29.1.0)

The same test produces a different result:

it('parses gitlab custom preset file with name and params', () => {
  expect(presets.parsePreset('gitlab>some/group/configs:presetfile/presetname(1, monday)')).toEqual({
    packageName: 'some/group/configs',
+   "packageTag": undefined,
    params: ["1","monday"],
-   "presetName": "schedules/weekly",
-   "presetPath": undefined,
+   "presetName": "weekly",
+   "presetPath": "schedules",
    presetSource: 'gitlab',
  });
});

https://github.com/renovatebot/renovate/pull/11565 definitely broke it but I haven't looked at the regex closely yet, just a heads up πŸ™‚

on Azure DevOps Server (?) can be removed from the title I'd say πŸ˜…

viceice commented 2 years ago

I kept the whole pattern: local>A2B CD/A2B_Renovate

Spaces are evil?

Yes, they where supported by accident.

viceice commented 2 years ago

@vntw can you try to removbe the space: gitlab>some/group/configs:presetfile/presetname(1,monday)

timmkrause commented 2 years ago

I kept the whole pattern: local>A2B CD/A2B_Renovate

Spaces are evil?

Yes, they where supported by accident.

This implies that it won't come back?

viceice commented 2 years ago

No decision yet, waiting for some other maintainer reponses. But spaces seems to be bad practice anyway to me πŸ€·β€β™‚οΈ

timmkrause commented 2 years ago

No decision yet, waiting for some other maintainer reponses. But spaces seems to be bad practice anyway to me πŸ€·β€β™‚οΈ

I agree with you. But at least some platforms do allow it though and it's questionable if an unspecifiale number of users which may not be in full control of their ("legacy") project names (organizational constraints, conventions etc.) should be locked out.

Personally I see this as a chance/reason to get rid of it.

Would an error message make sense in case it stays?

rarkins commented 2 years ago

I don't think anyone's forced to add presets to "legacy" projects though are they? So then the only scenario would be if someone has an org-wide policy that they must use spaces. Or have I misunderstood?

vntw commented 2 years ago

@vntw can you try to removbe the space: gitlab>some/group/configs:presetfile/presetname(1,monday)

Unfortunately not, it isn't related to spaces and/or parameters, they are parsed before any regexp is executed (which is what was changed in the mentioned commit, parameters weren't touched): https://github.com/renovatebot/renovate/blob/4aa5b52992c5b26b4b91ced4a589e61045784c6d/lib/config/presets/index.ts#L103-L109

Previously, for my preset string, a simpler regexp was used to define the various parts: https://github.com/renovatebot/renovate/blob/e09618f5305bd513f578b63181aabda19f6a8bb2/lib/config/presets/index.ts#L151-L161

See https://regex101.com/r/Dn5LhW/1 and how some/group/configs: is matched. presetPath will then be undefined and presetName will be str.slice(packageName.length + 1); which equals to presetfile/presetname. Also visible in the diff I posted earlier how this changes the preset parts.

Since 29.1.0 there's only one regexp and it doesn't account for slashes in presetName and assumes it's always part of the presetPath which didn't get defined in the 29.0.1 else case: https://github.com/renovatebot/renovate/blob/4aa5b52992c5b26b4b91ced4a589e61045784c6d/lib/config/presets/index.ts#L159-L173

I don't think this is even possible, it doesn't know where the path starts and the presetName (with possible slashes) ends. See https://regex101.com/r/PXc7TE/1 for an example and how the previous presetName with slashes is now separated into presetPath and presetName.

I'm not sure what to do here, revert the else case somehow (but still support #…), right now these kind of preset strings are unfortunately broken. I can open a new issue if this issue should only pertain to problems with spaces in preset names.

Thanks for initially looking into it though πŸ™‚

viceice commented 2 years ago

I think we won't revert that, a slash is also a bad char for JavaScript identifiers, so should be avoided.

So simply replace that by a dash or something else which is supported. πŸ€·β€β™‚οΈ

vntw commented 2 years ago

Sorry I don't get that. I'm trying to pass presetfile/presetname as the filePreset so that fetchPreset can split at /, read presetfile.json and access the presetname key:

https://github.com/renovatebot/renovate/blob/4aa5b52992c5b26b4b91ced4a589e61045784c6d/lib/config/presets/gitlab/index.ts#L66-L75

https://github.com/renovatebot/renovate/blob/4aa5b52992c5b26b4b91ced4a589e61045784c6d/lib/config/presets/util.ts#L23

https://github.com/renovatebot/renovate/blob/4aa5b52992c5b26b4b91ced4a589e61045784c6d/lib/config/presets/util.ts#L61-L62

Isn't that how it's supposed to work? Right now it's not possible to have a / in filePreset to define a presetName in the file from what I can see.

Edit: As an example, this is what my preset file looks like and why I'd like to access a preset name:

presetfile.json

{
  "$schema": "https://docs.renovatebot.com/renovate-schema.json",
  "presetname": {
    "description": "Daily schedule",
    "schedule": "…"
  },
  "otherpresetname": {
    "description": "Monthly schedule with arg0…",
    "schedule": "…"
  }
}
viceice commented 2 years ago

Ok, sorry, i misunderstand. It really looks like a regession of #11565.

Before we hat the double slash as delimitter for preset paths: local>abc/foo//path/xyz, so local>abc/foo:filename/xyz worked as subpreset.

I saw the docs where a little unclear about that and also we probably had no test for that, so the subpreset feature got's silently removed.

I think #8724 is also related.

@rarkins Should we revert #11565, as it totally broke subpresets. Can't remember if this was intended in a minor update.

rarkins commented 2 years ago

I guess if we can't "roll forward" then we should roll back.

viceice commented 2 years ago

Maybe we can roll forward and move the path back to allow this: local>org/repo//path/to/file:subpreset#1.5.4

viceice commented 2 years ago

and local>org/repo:filename/sub/preset#1.2.3

rarkins commented 2 years ago

I've prepared the revert in #12725 but unfortunately that too would be breaking for anyone who's added preset tags already.

rarkins commented 2 years ago

Still attempting to "roll forward", I think this PR fixes @timmkrause's original problem in this issue: #12726

rarkins commented 2 years ago

@viceice I'm not seeing any quick way forward with the sub-presets. Was your idea to support them exactly as they were before, or require a slight change in syntax?

viceice commented 2 years ago

I'm not sure, but my idea was to revert the new syntax partially and hope we don't cause too many issues.

I'll try to open a PR tomorrow morning. If I'm not able to fix it, we probably need to revert in a major bump?

renovate-release commented 2 years ago

:tada: This issue has been resolved in version 29.12.6 :tada:

The release is available on:

Your semantic-release bot :package::rocket:

vntw commented 2 years ago

Sorry for the noise but thank you for fixing this so quickly ❀️