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.

Sign up for GitHub

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

Jump to bottom

feat: Introduce requirePoetryLockFile flag #728

Merged
merged 1 commit into from
Oct 18, 2022
Merged

feat: Introduce requirePoetryLockFile flag #728

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,6 +131,15 @@ custom:
usePoetry: false
```

Be aware that if no `poetry.lock` file is present, a new one will be generated on the fly. To help having predictable builds,
you can set the `requirePoetryLockFile` flag to true to throw an error when `poetry.lock` is missing.

```yaml
custom:
pythonRequirements:
requirePoetryLockFile: false
```

### Poetry with git dependencies

Poetry by default generates the exported requirements.txt file with `-e` and that breaks pip with `-t` parameter
Expand Down
1 change: 1 addition & 0 deletions index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ class ServerlessPythonRequirements {
pipCmdExtraArgs: [],
noDeploy: [],
vendor: '',
requirePoetryLockFile: false,
},
(this.serverless.service.custom &&
this.serverless.service.custom.pythonRequirements) ||
Expand Down
26 changes: 21 additions & 5 deletions lib/poetry.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,12 +21,28 @@ async function pyprojectTomlToRequirements(modulePath, pluginInstance) {
generateRequirementsProgress = progress.get(
'python-generate-requirements-toml'
);
generateRequirementsProgress.update(
'Generating requirements.txt from "pyproject.toml"'
);
log.info('Generating requirements.txt from "pyproject.toml"');
}

const emitMsg = (msg) => {
if (generateRequirementsProgress) {
generateRequirementsProgress.update(msg);
log.info(msg);
} else {
serverless.cli.log(msg);
}
};

if (fs.existsSync('poetry.lock')) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's hide this check behind the requirePoetryLockFile, there's no need to check if for users that don't even use this flag

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There are two different accurate logging messages with that check though. This is the thing that brought me to open this PR: When I started working with the serverless-python-requirements plugin and saw the former message, I was unsure what it was actually doing so I had to open the code to verify.

That being said, tell me what you want me to do. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @FinchPowers, I'm not sure I understand. Why do you think it makes sense to check it for each user, even the ones that don't use the introduced flag? Maybe I'm missing something here

Copy link
Contributor Author

@FinchPowers FinchPowers Oct 9, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's to provide accurate logging. Let's say the flag does not exist, that would still leave this

if (fs.existsSync('poetry.lock')) {
  emitMsg('Generating requirements.txt from poetry.lock');
} else {
  emitMsg('Generating poetry.lock and requirements.txt from pyproject.toml');
}

The distinction is important. In the former case, you end up with a predictable requirements.txt which means that you have a repeatable build. In the later, you don't, it depends on what time the command was run and what was available of that time. (Unless all dependencies are locked to a specific version in pyproject.toml, which is rarely the case.) So if you rebuild to troubleshoot, you may endup with different dependencies and not be able to reproduce.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@pgrzesik Standing by: Tell me what you would like me to do. :)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @FinchPowers 👋 Sorry for not responding sooner, I was on a short vacation for pretty much the whole last week.

That makes a lot of sense, thanks for the explanation 👍

emitMsg('Generating requirements.txt from poetry.lock');
} else {
serverless.cli.log('Generating requirements.txt from pyproject.toml...');
if (options.requirePoetryLockFile) {
throw new serverless.classes.Error(
'poetry.lock file not found - set requirePoetryLockFile to false to ' +
FinchPowers marked this conversation as resolved.
Show resolved Hide resolved
'disable this error',
'MISSING_REQUIRED_POETRY_LOCK'
);
}
emitMsg('Generating poetry.lock and requirements.txt from pyproject.toml');
}

try {
Expand Down
20 changes: 20 additions & 0 deletions test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1634,3 +1634,23 @@ test('py3.7 can ignore functions defined with `image`', async (t) => {

t.end();
});

test('poetry py3.7 fails packaging if poetry.lock is missing and flag requirePoetryLockFile is set to true', async (t) => {
copySync('tests/poetry', 'tests/base with a space');
process.chdir('tests/base with a space');
removeSync('poetry.lock');

const path = npm(['pack', '../..']);
npm(['i', path]);
const stdout = sls(['package'], {
env: { requirePoetryLockFile: 'true', slim: 'true' },
noThrow: true,
});
t.true(
stdout.includes(
'poetry.lock file not found - set requirePoetryLockFile to false to disable this error'
),
'flag works and error is properly reported'
);
t.end();
});
1 change: 1 addition & 0 deletions tests/poetry/serverless.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ custom:
slimPatterns: ${file(./slimPatterns.yml):slimPatterns, self:custom.defaults.slimPatterns}
slimPatternsAppendDefaults: ${env:slimPatternsAppendDefaults, self:custom.defaults.slimPatternsAppendDefaults}
dockerizePip: ${env:dockerizePip, self:custom.defaults.dockerizePip}
requirePoetryLockFile: ${env:requirePoetryLockFile, false}
defaults:
zip: false
slimPatterns: false
Expand Down