Improved/consistent artifact constraints handling

[rarkins]

What would you like Renovate to be able to do?

Improve constraints handling so that they are read as late as possible during updateArtifacts() and so that configured constraints take priority.

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

Today, managers can return constraints as an output of the extract process. e.g. it could be a node or python language constraint, or a pnpm manager constraint.

In theory these can be used as part of the lookup process, such as restricting packages to a particular python validity. So far we underuse these though - python is the only one we support - but in future we'll use it more.

Additionally we also allow configuration of constraints by config too. Such constraints should usually not be needed but if they are present then they should override the constraints we detect from repos.

Constraints are also used during artifact updating to pick which version of node/npm/python/pipenv/etc are in use.

We need to make some changes to how we detect and use constraints.

First of all, we should change the extract output so that it returns an internal field like extractedConstraints instead of constraints. That way we can distinguish between configured constraints vs extracted ones.

The lookup procedure should prioritise config.constraints.x over config.extractedConstraints.x if both are present.

Finally, during the updateArtifacts() phase:

• Use config.constraints.x if present, otherwise
• Parse the package files/other files again in the current branch to extract constraints. By doing this it means that if the constraint itself was updated in the same PR, we will run the right version.

Today our manager handling of constraints is a bit inconsistent - some do it close to the description above, while others do not.

Possibly this issue could be implemented manager-by-manager.

Is this a feature you are interested in implementing yourself?

No

[PhilipAbed]

according to my initial shallow analysis : pnpm confirmed solved in #14660 by me. poetry confirmed solved by rhys on january 2022 docker gradle-wrapper, confirmed solved by kriese on april 2022

yarn needs to be resolved :

  const yarnCompatibility = yarnUpdate
      ? yarnUpdate.newValue
      : config.constraints?.yarn;

npm config.constraint is being set in the extraction:

   const npmToolConstraint: ToolConstraint = {
      toolName: 'npm',
      constraint: config.constraints?.npm,
    };

node seems like config.constraint's is the last to be taken into consideration

  const constraint =
    (await getNodeFile(getSiblingFileName(packageFile, '.nvmrc'))) ||
    (await getNodeFile(getSiblingFileName(packageFile, '.node-version'))) ||
    getPackageJsonConstraint(config);

docker go needs debugging and deep dive to understand we seem to create our own constraint using this:

  if (
        line.startsWith('go ') &&
        semver.validRange(line.replace('go ', ''))
      ) {
        constraints.go = line.replace('go ', '^');
      }

helmv3 - seems like there's no extraction at all for any constraint, even though i can see clearly there is an example of : chart.yaml where it has

 env:
  HELM_VERSION: v3.8.1

not sure we need to change anything since we dont even try to extract the version from the chart.yaml

pipenv lerna jb

i will look deeper into each one to make sure.

[viceice]

npm is filling it for all npm base managers in extract phase.

I've a PR open, which changes some yarn artifact handling, so please start with other managers to not get conflicted. 😉

[PhilipAbed]

in poetry, there are 2 constraints, 1) Tag Constraint which is Python, We extract the python version from extract.ts

       if (is.nonEmptyString(pyprojectfile.tool?.poetry?.dependencies?.python)) {
           constraints.python = pyprojectfile.tool?.poetry?.dependencies?.python;
       } 

 and then in the update if its not found, we try to pick it up from the lock file
 ```
 const data = parse(existingLockFileContent) as PoetryLock;
     if (is.string(data?.metadata?.['python-versions'])) {
       return data?.metadata?.['python-versions'];
     }
 ```
 Note: No Mention of Config.Constraints.Python!

2) Poetry Constraint which is the actual package manager: it is doing exactly what we want it to do, a lazy extraction!

const constraint = config.constraints?.poetry || getPoetryRequirement(newPackageFileContent);

I Stumbled upon a bug i think... When trying to extract in the Artifacts.ts

according to this image, my file has poetry-core but the code is expecting poetry_core which is why my example test failed. check this link out poetry-core,

Summary: Poetry is closed from this task's point of view. but a bug was found :D should we open an issue for this?

[viceice]

in poetry, there are 2 constraints, 1) Tag Constraint which is Python, We extract the python version from extract.ts

       if (is.nonEmptyString(pyprojectfile.tool?.poetry?.dependencies?.python)) {
           constraints.python = pyprojectfile.tool?.poetry?.dependencies?.python;
       } 

 and then in the update if its not found, we try to pick it up from the lock file
 ```
 const data = parse(existingLockFileContent) as PoetryLock;
     if (is.string(data?.metadata?.['python-versions'])) {
       return data?.metadata?.['python-versions'];
     }
 ```
 Note: No Mention of Config.Constraints.Python!

2) Poetry Constraint which is the actual package manager: it is doing exactly what we want it to do, a lazy extraction!

const constraint = config.constraints?.poetry || getPoetryRequirement(newPackageFileContent);

I Stumbled upon a bug i think... When trying to extract in the Artifacts.ts

according to this image, my file has poetry-core but the code is expecting poetry_core which is why my example test failed. check this link out poetry-core,

Summary: Poetry is closed from this task's point of view. but a bug was found :D should we open an issue for this?

i think you can open a PR to completely remove from extract and add missing parts to update artifacts.

[PhilipAbed]

i think you can open a PR to completely remove from extract and add missing parts to update artifacts.

The extract part is only for Python not for poetry, and it has nothing to do with config.constraints why would we want to move it? its also just more complex since in the extraction we have the file parsed its just 1 liner, but if we go to the update function there's nowhere to get the pyproject.toml file content, i have only the newPackageFileContent which is after the update. i dont see any CWD to work with even if i wanted to get the constraint from the old pyproject.toml.

are you sure you want to do this? i dont see a reason to

[viceice]

i think you can open a PR to completely remove from extract and add missing parts to update artifacts.

The extract part is only for Python not for poetry, and it has nothing to do with config.constraints why would we want to move it? its also just more complex since in the extraction we have the file parsed its just 1 liner, but if we go to the update function there's nowhere to get the pyproject.toml file content, i have only the newPackageFileContent which is after the update. i dont see any CWD to work with even if i wanted to get the constraint from the old pyproject.toml.

are you sure you want to do this? i dont see a reason to

This should be moved to lazy resolving in artifacts https://github.com/renovatebot/renovate/blob/6c7e79fbe3fe7f78987c168ee01de209d5f7a233/lib/modules/manager/poetry/extract.ts#L133-L135

As it's setting config.constraints for python. It should be moved after this https://github.com/renovatebot/renovate/blob/6c7e79fbe3fe7f78987c168ee01de209d5f7a233/lib/modules/manager/poetry/artifacts.ts#L33-L36

you need to move the newPackageFileContent parsing out of getPoetryRequirement and pass the PoetryFile object to getPoetryRequirement and getPythonConstraint

[PhilipAbed]

oh its actually parsing the new file content instead of the old one!

and what about the poetry bug with poetry-core - poetry_core underscore? should i fix that on the way? just add replace - to _?

[viceice]

oh its actually parsing the new file content instead of the old one!

yes, that way we also catch python updates and use the required new version 😉

and what about the poetry bug with poetry-core - poetry_core underscore? should i fix that on the way? just add replace - to _?

I think this should be solved in a separate PR before or after refactor

[rarkins]

FYI we use constraints.python during datasource lookups, so it needs to remain. However it should be re-calculated prior to artifacts updating in case it's been upgraded in the same PR.

[PhilipAbed]

@rarkins but it's overriding the "constraints.python" from config right?

[rarkins]

Good catch. Maybe we need to separate the concept of config constraints (which overrides everything) from detected constraints, Eg a new internal field

[PhilipAbed]

or we can just override only in case its empty, instead of a big change

[viceice]

I'll refactor npm now to partial fix #15511

[viceice]

or we can just override only in case its empty, instead of a big change

no. we want to lazy extract on update-artifacts if not user supplied.

@rarkins what about extractedConstraints field?

[PhilipAbed]

@viceice you want to do lazy extract on update-artifacts @rarkins you say we must keep extraction on extract phase because its being used in lookup. this doesn't make sense to me, i have the code ready almost, but not sure what to do with this, and need to fix tests, but need to know how should i continue, or should i try to change design?

[viceice]

@PhilipAbed We need both. for poetry we need to extract the python version to a new field, eg extractedContraints which can be used by datasource (cuurently required for pypi datasource). Then on update-artifacts we use normal contraints (can be set by user) or lazy extract again (could be updated in same branch).

[hasanwhitesource]

@viceice Coming a bit late to work on this, but to be clear on what was agreed upon :

1. In poetry/artifacts.ts we are going to pass the poetryFile object to getPoetryRequirement and getPythonConstraint?
2. Introduce a new field in the config called extractedContraints that will hold the tool.poetry.dependencies.python which will be checked at updateArtifacts?

[PhilipAbed]

FYI we use constraints.python during datasource lookups

hi rhys, im looking with hasan but i cant find what you mean here, i cant find how it is being used, can you elaborate?

[rarkins]

@PhilipAbed this logic here: https://github.com/renovatebot/renovate/blob/4fe896ec03406ae9a034da8f8d51cb6ed579168e/lib/modules/datasource/index.ts#L378-L398

It's designed to be generic for future use, and only filters if there's both input constraints (e.g. from manager.extract logic) as well as release constraints (from the datasource).

pypi is currently the only datasource to return constraints: https://github.com/renovatebot/renovate/blob/4fe896ec03406ae9a034da8f8d51cb6ed579168e/lib/modules/datasource/pypi/index.ts#L160-L163

In short, if you remove constraints from any of the python manager extracts, then this functionality will stop working.

[PhilipAbed]

renovate/lib/modules/datasource/index.ts

That means this is for ALL Package managers right? So in all of them, you need to extract constraints in Extraction phase and also in artifact phase?

that is not what we did in pnpm as far as i remember we removed it from extraction O.o

[rarkins]

Currently, constraints may be used in either datasource lookup phase (to check compatibility), or in artifacts update phase (to pick the best version of the tool to use).

In practice, constraints are only used by the pypi datasource.

What I suggest is this:

• Keep the field constraints for only user-provided constraints
• Change all extract functions to return a new internal field/object extractedConstraints instead of constraints
• Merge extractedConstraints with constraints during datasource lookup (constraints take precedence)
• Discard extractedConstraints after that (during artifacts phase we should "extract" them again in case they changed during update phase)

[PhilipAbed]

i had a chance to work on gomod, we extract constraints.go from go.mod file and then we set it in the extraction then we use it in manager/gomod/artifacts.ts as the actual constraint, ill open another issue for this.

[rarkins]

@PhilipAbed that sounds like exactly the type of thing we need to fix as part of this issue. Any constraint from our extract phase should not be used once datasource lookups are done. Constraints should be recalculated during artifacts phase

[MaronHatoum]

Question:

go.mod file contains: go 1.17

and renovate.json file contains a constraint: go 1.16.14

when running Renovate with a constraint lower than the go.mod file, go will fail with this msg: go mod tidy: go.mod file indicates go 1.17, but maximum supported version is 1.16

Is it the user's responsibility (Is this the expected behavior)? or should we handle this?

[rarkins]

Yes, it's the user's responsibility and we should run what they tell us to run (1.16.4)