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

fix: tag pipenv version to install #7688

Merged
merged 39 commits into from
Nov 12, 2020
Merged

fix: tag pipenv version to install #7688

merged 39 commits into from
Nov 12, 2020

Conversation

javulticat
Copy link
Contributor

@javulticat javulticat commented Nov 10, 2020

Changes:

Installs a known working pipenv release to avoid Renovate breaking when a pipenv release is bugged or includes breaking changes.

Context:

As maintainers of a tool to help with dependency management, I'm sure you can appreciate the importance of reproducible builds. Unfortunately, the build being used by Renovate to run pipenv lock is not reproducible, because it is always installing the latest version of pipenv. As of last week with the 2020.11.4 release of pipenv, pipenv lock has been broken, which has prevented Renovate from working on projects that use pipenv. By tagging a specific pipenv release, we can ensure that Renovate's pipenv build is reproducible and will not be blocked by upstream changes in pipenv.

Documentation (please check one with an [x])

  • I have updated the documentation, or
  • No documentation update is required

How I've tested my work (please tick one)

I have verified these changes via:

  • Code inspection only, or
  • Newly added unit tests, or
  • Unit tests + ran on a real repository

@javulticat javulticat changed the title Tag pipenv version to install fix: Tag pipenv version to install Nov 10, 2020
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
@viceice viceice marked this pull request as draft November 10, 2020 01:35
@javulticat javulticat marked this pull request as ready for review November 10, 2020 19:21
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
@viceice
Copy link
Member

viceice commented Nov 11, 2020

constraints should be read in extraction phase too, see poetry

const constraints: Record<string, any> = {};
// https://python-poetry.org/docs/pyproject/#poetry-and-pep-517
if (
pyprojectfile['build-system']?.['build-backend'] === 'poetry.masonry.api'
) {
constraints.poetry = pyprojectfile['build-system']?.requires.join(' ');
}
if (is.nonEmptyString(pyprojectfile.tool?.poetry?.dependencies?.python)) {
constraints.python = pyprojectfile.tool?.poetry?.dependencies?.python;
}
return {
deps,
registryUrls: extractRegistries(pyprojectfile),
constraints,
};

lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
lib/manager/pipenv/artifacts.ts Outdated Show resolved Hide resolved
@javulticat
Copy link
Contributor Author

constraints should be read in extraction phase too, see poetry

const constraints: Record<string, any> = {};
// https://python-poetry.org/docs/pyproject/#poetry-and-pep-517
if (
pyprojectfile['build-system']?.['build-backend'] === 'poetry.masonry.api'
) {
constraints.poetry = pyprojectfile['build-system']?.requires.join(' ');
}
if (is.nonEmptyString(pyprojectfile.tool?.poetry?.dependencies?.python)) {
constraints.python = pyprojectfile.tool?.poetry?.dependencies?.python;
}
return {
deps,
registryUrls: extractRegistries(pyprojectfile),
constraints,
};

@viceice I'm not sure I understand how the constraints defined here are then being used. For example, in poetry/artifact.ts, there is still this code to determine the Python constraint that doesn't seem to use the data extracted in the place you highlighted.

function getPythonConstraint(
existingLockFileContent: string,
config: UpdateArtifactsConfig
): string | undefined | null {
const { constraints = {} } = config;
const { python } = constraints;
if (python) {
logger.debug('Using python constraint from config');
return python;
}
try {
const data = parse(existingLockFileContent);
if (data?.metadata?.['python-versions']) {
return data?.metadata?.['python-versions'];
}
} catch (err) {
// Do nothing
}
return undefined;
}

rarkins
rarkins previously approved these changes Nov 11, 2020
@viceice
Copy link
Member

viceice commented Nov 11, 2020

constraints should be read in extraction phase too, see poetry

const constraints: Record<string, any> = {};
// https://python-poetry.org/docs/pyproject/#poetry-and-pep-517
if (
pyprojectfile['build-system']?.['build-backend'] === 'poetry.masonry.api'
) {
constraints.poetry = pyprojectfile['build-system']?.requires.join(' ');
}
if (is.nonEmptyString(pyprojectfile.tool?.poetry?.dependencies?.python)) {
constraints.python = pyprojectfile.tool?.poetry?.dependencies?.python;
}
return {
deps,
registryUrls: extractRegistries(pyprojectfile),
constraints,
};

@viceice I'm not sure I understand how the constraints defined here are then being used. For example, in poetry/artifact.ts, there is still this code to determine the Python constraint that doesn't seem to use the data extracted in the place you highlighted.

function getPythonConstraint(
existingLockFileContent: string,
config: UpdateArtifactsConfig
): string | undefined | null {
const { constraints = {} } = config;
const { python } = constraints;
if (python) {
logger.debug('Using python constraint from config');
return python;
}
try {
const data = parse(existingLockFileContent);
if (data?.metadata?.['python-versions']) {
return data?.metadata?.['python-versions'];
}
} catch (err) {
// Do nothing
}
return undefined;
}

Poetry constraint is currently not used but we can add pipenv version from project file (if added) to constraints, so your new add function will automatically use it. You've already added the check in the first if condition. 😉

@javulticat
Copy link
Contributor Author

@viceice I took a shot at adding the requested changes around constraint extraction, let me know if it is what you were looking for!

cc @rarkins

@rarkins rarkins changed the title fix: Tag pipenv version to install fix: tag pipenv version to install Nov 12, 2020
@rarkins rarkins merged commit 63c3581 into renovatebot:master Nov 12, 2020
@renovate-release
Copy link
Collaborator

🎉 This PR is included in version 23.82.1 🎉

The release is available on:

Your semantic-release bot 📦🚀

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 15, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants