feat: Support individual packaging with poetry

[BrandonLWhite]

This change allows use of Poetry in conjunction with the package.individually option.
Unit test coverage added.

Should resolve:
#654
#576

[pgrzesik]

Hey @BrandonLWhite - could you look into formatting issues and rebase your branch on top of current master? Thanks in advance 🙇

[BrandonLWhite]

Hey @BrandonLWhite - could you look into formatting issues and rebase your branch on top of current master? Thanks in advance 🙇

Done!

[BrandonLWhite]

I will look into the ESLint failures

[BrandonLWhite]

ESLint failure should be fixed now.

[pgrzesik]

Thanks @BrandonLWhite - I have a few questions, it looks good overall though 👍

[pgrzesik]

Why is that change needed here?

[BrandonLWhite]

Since the source files for this project are located in ./lib/ , this .gitignore was causing any changes to the JS files to be ignored for git purposes. It seemed quite peculiar to me that a project would .gitignore it's own source folder.

[pgrzesik]

Oh that's right - I guess it would be better to just remove this line instead of commenting it

[pgrzesik]

This seems like a quite weird pattern to me - could you explain the reasoning behind adding logic like that?

[BrandonLWhite]

Briefly, because I needed to reuse the pyprojectTomlToRequirements function and it was originally written to leverage JS this binding.

The functions used in this block:

      return BbPromise.bind(this)
        .then(pipfileToRequirements)
        .then(_pyprojectTomlToRequirements)
        .then(addVendorHelper)
        .then(installAllRequirements)
        .then(packRequirements)
        .then(setupArtifactPathCapturing);

are implemented to use the this binding pattern, where dependencies/args are passed using the this object. Elsewhere in the project, functions are implemented using an explicit pluginInstance context argument.
My understanding is that the former technique is widely considered a legacy JS pattern, whereas the latter is a more acceptable modern practice.

I opted to convert pyprojectTomlToRequirements to use the latter explicit context argument so that I could invoke it from a function that was already being called with such a context (pluginInstance) arg.
In order to make the resulting pyprojectTomlToRequirements compatible with this BbPromise call async chain, I needed to convert the local this instance into a function arg, in addition to passing an empty string for the modulePath argument (since this call context is for the project root directory).
The lambda function I used achieves this adaption.

[pgrzesik]

Thanks for clarification - instead of adding this temp var _pyprojectTomlToRequirements let's just add an inline function - I don't think we need this one-of temporary variable

[pgrzesik]

Did you validate how that behaves when there's multiple functions packaged individually at the same time? Are they being packaged in parallel or not?

[BrandonLWhite]

No, I was not aware of this mechanism. I will try to investigate and get back to you.

[BrandonLWhite]

I conducted an experiment with multiple functions packaged individually. It appears that this plugin is written to package the functions sequentially and does not support parallel packaging:

From installAllRequirements.js:

    for (const f of filteredFuncs) {
      if (!get(f, 'module')) {
        set(f, ['module'], '.');
      }

      // If we didn't already process a module (functions can re-use modules)
      if (!doneModules.includes(f.module)) {
        const reqsInstalledAt = await installRequirementsIfNeeded(
          f.module,
          f,
          this
        );
...

Note how the await installRequirementsIfNeeded is inside of the for loop. This will prevent parallel execution of the packaging of the functions.

[pgrzesik]

Thanks for checking that 🙇

[pgrzesik]

Thanks @BrandonLWhite - I have one minor comment and it seems like the CI checks are failing - could you please look into that? Otherwise I think we will be good to go 💯

[BrandonLWhite]

Code review changes are complete.

Regarding the failed [[Linux] Node.js v14: Lint, Eventual Commitlint, Eventual Changelog, Formatting & Unit tests (2.7)] test, I am seeing that this was a transient error:

...

  File "/usr/local/lib/python2.7/site-packages/pip/_vendor/urllib3/response.py", line 541, in read
    raise IncompleteRead(self._fp_bytes_read, self.length_remaining)
  File "/usr/lib64/python2.7/contextlib.py", line 35, in __exit__
    self.gen.throw(type, value, traceback)
  File "/usr/local/lib/python2.7/site-packages/pip/_vendor/urllib3/response.py", line 455, in _error_catcher
    raise ProtocolError("Connection broken: %r" % e, e)
ProtocolError: ("Connection broken: error(104, 'Connection reset by peer')", error(104, 'Connection reset by peer'))

I ran the tests locally and they past (with Node 14 and Python 2.7 as specified in the test). I also ran the GitHub actions in my forked repo and they all passed:
https://github.com/BrandonLWhite/serverless-python-requirements/runs/5469863884?check_suite_focus=true

So I'm hopeful that if you run the GitHub Actions this time they will pass.

[pgrzesik]

Thanks @BrandonLWhite - sorry for closing and reopening the PR but it seemed to be the only way to actually force Github Actions to pick up new changes and re-run the pipeline 😕

[pgrzesik]

Thanks @BrandonLWhite - looks good overall, I have one comment and let's also see that all tests are passing

[pgrzesik]

Thank you @BrandonLWhite 🙇 Looks good 👍