feat: Introduce `requirePoetryLockFile` flag #728

FinchPowers · 2022-09-28T01:13:37Z

Distinct logging whether the requirements.txt file is generated from poetry.lock vs pyproject.toml.
New boolean flag: requirePoetryLockFile - When set to true, fail the build when poetry.lock is missing. This helps with creating reproducible builds where you want to have a fixed poetry.lock file instead of one generated on the fly.

Follow up to #639

FinchPowers · 2022-09-28T01:14:13Z

lib/poetry.js

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


I am less certain about this part. I extracted what looked like the logging logic.

pgrzesik

Hey @FinchPowers, thanks a lot for proposing this. I think the idea is good, however, I have a few comments and it seems like the CI builds are failing. Could you please look into that? Thanks!

pgrzesik · 2022-09-29T21:30:07Z

lib/poetry.js

+  }
+
+  try {
+    await spawn("test", ["-f", "poetry.lock"])


Is that command cross-platform?

I guess not! :) I'll have a look.

thanks 🙇

pgrzesik · 2022-09-29T21:31:29Z

lib/poetry.js

+    emitMsg('Generating requirements.txt from poetry.lock...');
+  } catch (e) {
+    if (options.requirePoetryLockFile) {
+      throw new Error(


Let's not throw such errors, use the Error class passed from the Serverless class (serverless.classes.Error).

pgrzesik · 2022-09-29T21:32:10Z

lib/poetry.js

+      );
+    }
+    emitMsg(
+      'Generating poetry.lock and requirements.txt from pyproject.toml...'


Let's not put the dots at the end - it can update an interactive spinner which already indicates that something is happening

pgrzesik · 2022-09-29T21:33:38Z

And I forgot - it would be great to cover that functionality with a test case 💯

FinchPowers · 2022-10-01T01:47:47Z

@pgrzesik Thanks for the pointers, I've updated the code.

For testing, I can, but I'll need some pointers. Is it using a testing framework? From a quick glance at the code it seems to be a custom implementation.

How do you suggest I approach this? I see two options:

Use a filesystem mocker to hide tests/poetry/poetry.lock and invoke the package on it with requirePoetryLockFile set to true.

or

At testing time, copy tests/poetry to tests/poetry-no-lock, remove the lock file from it and invoke the package on it with requirePoetryLockFile set to true.

pgrzesik · 2022-10-01T08:26:43Z

Hey @FinchPowers, thanks a lot 🙇 As for testing, it's using tape, but it's pretty barebones as you've noticed and kinda looks like a custom implementation. I don't have much experience with it as well, but I think it would be best to avoid mocking, so something similar to your suggestion 2 would be best 👍 I don't think you will have to copy poetry anywhere, but you should be able to just use that "fixture" as each test is run separately and in isolation (see setup and teardown methods). If you have any questions, please let me know 💯

pgrzesik

Hey @FinchPowers, it looks great overall 🙌 I have only two minor suggestions, please let me know what do you think 🙇

pgrzesik · 2022-10-08T08:36:33Z

lib/poetry.js

+    }
+  };
+
+  if (fs.existsSync('poetry.lock')) {


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

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. :)

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

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.

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

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 👍

lib/poetry.js

* Distinct logging whether the requirements.txt file is generated from poetry.lock vs pyproject.toml. * New boolean flag: requirePoetryLockFile - When set to true, fail the build when poetry.lock is missing. This helps with creating reproducible builds where you want to have a fixed poetry.lock file instead of one generated on the fly.

pgrzesik

Looking great @FinchPowers, thanks a lot 🙇 Let's wait for tests to pass and I'll merge the PR

FinchPowers commented Sep 28, 2022

View reviewed changes

pgrzesik requested changes Sep 29, 2022

View reviewed changes

FinchPowers force-pushed the feature/log-and-require-poetry-lock branch from cf627ff to 166ae65 Compare October 1, 2022 01:12

FinchPowers force-pushed the feature/log-and-require-poetry-lock branch 3 times, most recently from 794a646 to 24e2d2c Compare October 5, 2022 01:38

FinchPowers requested a review from pgrzesik October 5, 2022 01:38

pgrzesik reviewed Oct 8, 2022

View reviewed changes

pgrzesik added the enhancement label Oct 8, 2022

FinchPowers force-pushed the feature/log-and-require-poetry-lock branch from 24e2d2c to 3ddf189 Compare October 14, 2022 01:16

FinchPowers requested a review from pgrzesik October 14, 2022 01:20

pgrzesik approved these changes Oct 17, 2022

View reviewed changes

pgrzesik changed the title ~~Enhance poetry related logging and add new flag~~ feat: Introduce requirePoetryLockFile flag Oct 18, 2022

pgrzesik merged commit e81d9e1 into serverless:master Oct 18, 2022

stackspotadmin mentioned this pull request Feb 11, 2024

[Snyk] Upgrade serverless-python-requirements from 5.4.0 to 6.0.1 stack-spot/stackspot-python-stack#22

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: Introduce `requirePoetryLockFile` flag #728

feat: Introduce `requirePoetryLockFile` flag #728

FinchPowers commented Sep 28, 2022 •

edited

FinchPowers Sep 28, 2022

pgrzesik left a comment

pgrzesik Sep 29, 2022

FinchPowers Oct 1, 2022

pgrzesik Oct 1, 2022

pgrzesik Sep 29, 2022

pgrzesik Sep 29, 2022

pgrzesik commented Sep 29, 2022

FinchPowers commented Oct 1, 2022

pgrzesik commented Oct 1, 2022

pgrzesik left a comment

pgrzesik Oct 8, 2022

FinchPowers Oct 8, 2022

pgrzesik Oct 9, 2022

FinchPowers Oct 9, 2022 •

edited

FinchPowers Oct 14, 2022

pgrzesik Oct 17, 2022

pgrzesik left a comment

feat: Introduce requirePoetryLockFile flag #728

feat: Introduce requirePoetryLockFile flag #728

Conversation

FinchPowers commented Sep 28, 2022 • edited

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik commented Sep 29, 2022

FinchPowers commented Oct 1, 2022

pgrzesik commented Oct 1, 2022

pgrzesik left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

FinchPowers Oct 9, 2022 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pgrzesik left a comment

Choose a reason for hiding this comment

feat: Introduce `requirePoetryLockFile` flag #728

feat: Introduce `requirePoetryLockFile` flag #728

FinchPowers commented Sep 28, 2022 •

edited

FinchPowers Oct 9, 2022 •

edited