feat(Packaging): Add patterns setting and deprecate include/exclude

[juanjoDiaz]

Closes: #8093

[codecov-io]

Codecov Report

Merging #8581 (2e9f0ed) into master (14f5743) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 2e9f0ed differs from pull request most recent head 000bca0. Consider uploading reports for the commit 000bca0 to get more accurate results

@@           Coverage Diff           @@
##           master    #8581   +/-   ##
=======================================
  Coverage   87.15%   87.16%           
=======================================
  Files         304      304           
  Lines       11467    11474    +7     
=======================================
+ Hits         9994    10001    +7     
  Misses       1473     1473           

Impacted Files	Coverage Δ
lib/configSchema.js	100.00% <ø> (ø)
lib/plugins/aws/provider.js	94.07% <ø> (ø)
lib/plugins/package/lib/packageService.js	96.00% <100.00%> (ø)
lib/plugins/package/package.js	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 14f5743...000bca0. Read the comment docs.

[medikoo]

@juanjoDiaz great thanks for that! Can you ensure that branch is up to date with master and that CI passes(?) I see that packing tests failed (those run through npm run integration-test-run-package)

[juanjoDiaz]

Rebased on top of master and fixed the broken test.

Let me know if there is anything else pending.

[medikoo]

@juanjoDiaz great thanks for that! It looks very promising, still see my comments.

I believe we need to narrow down scope of this purely, so it's purely about introduction of patterns, and doesn't touch any functionality that processes resolved patterns (this can be in scope of other PR)

[medikoo]

We need to also define it on functions level, here:

serverless/lib/plugins/aws/provider/awsProvider.js

Lines 961 to 968 in b12d565

	package: {
	type: 'object',
	properties: {
	artifact: { type: 'string' },
	exclude: { type: 'array', items: { type: 'string' } },
	include: { type: 'array', items: { type: 'string' } },
	individually: { type: 'boolean' },
	},

[medikoo]

New deprecation should always be added on top

[medikoo]

Let's title it New way to define packaging patterns

[medikoo]

Let's also update all the docs (in docs folder, all references to package.include and package.exclude - simply remove those references and document package.patterns)

Then also let's link here packaging docs section that documents package.patterns

[juanjoDiaz]

I can tackle this once we are happy with the implementation 🙂

[medikoo]

I suggest to add it in simpler way, with less changes to the file:

package.pattern can be treat as an alias (or rather suplement) to package.includes (note that currently in package.includes we support negation patterns)

So simple way to add recognition of patterns, and pave path for future removal of include and exclude could be:

1. Rename getIncludes to getPatterns
2. Upgrade signature of getPatterns to accept options object (that may contain include and pattern) property - update all invocations to function, so options are passed instead
3. In getPatterns, construct final patterns collection by ensuring that patterns in all cases are handled last, so in following order:
   1. service.include
   2. function.include
   3. service.pattern
   4. function.patterns

And that should be all changes I think needed for this functionality to work as expected - we should not interfere with the logic that processes patterns as returned by getPatterns

[juanjoDiaz]

That's essentially what I've done.

There are 3 functions that gather the patterns: resolveFilePathsAll, resolveFilePathsFunction and resolveFilePathsLayer`.

The three of them do the same:

    return this.getExcludes(funcPackageConfig.exclude, true)
      .then(exclude => {
        const params = { exclude, include: this.getIncludes(funcPackageConfig.include) };
        return params;
      })
      .then(params => this.excludeDevDependencies(params))
      .then(params => this.resolveFilePathsFromPatterns(params));

So I've abstracted that logic to a getPatterns function which calls getExcludes and getIncludes.
getIncludes does exactly as you suggested.
getExcludes only change is that it adds the dependencies from devDependencies, instead of doing a separate call to the function in zipService that would add it to the exclude property as I explained in a different comment.

Does that make sense?
I understand that you want to minimize changes, but I think that the code was getting a little bit spaghetti and the implementation in this PR really simplifies things.

[medikoo]

That's essentially what I've done.

Changes I see now, look as way more than needed to accomplish what's described above, and here we deal with really sensitive functionality, that's why I'm pushing for minimal needed changeset

I wouldn't touch getExcludes and I don't think any changes are needed in zipService. I've elaborated on that a bit more here: #8581 (comment)

Also, sorry I don't want to sound dismissive, in overall you've done a really great work.

[medikoo]

Let's configure this deprecation in initialization hook here:

serverless/lib/plugins/package/package.js

Line 57 in b12d565

this.hooks = {

Example on how it's done elsewhere:

serverless/lib/plugins/aws/package/compile/events/apiGateway/index.js

Line 235 in b12d565

if (

It will allow the deprecation to surface at earliest convenience (before any eventual error is reported)

[medikoo]

Why are there any changes in this file?

[juanjoDiaz]

This excludeDevDependencies method received the package config and updated the exclude property with the dev dependencies.

• It shouldn't be in zipService but in packageService (I didn't change it to don't make the review even more complicated.)
• The method is really tightly coupled with the include/exclude format. Now, it simple returns the list of devDependencies' patterns so the calling method can add them to the patterns in any way (future-proffness).

[medikoo]

Thanks for clarifications. Actually excludeDevDependencies doesn't really receive package config directly.

The actual packaging config is consumed in two places:

• serverless/lib/plugins/package/lib/packageService.js

  Line 21 in 8545268

  const packageIncludes = this.serverless.service.package.include || [];

• serverless/lib/plugins/package/lib/packageService.js

  Lines 197 to 201 in 8545268

  	const funcPackageConfig = functionObject.package || {};
  	return this.getExcludes(funcPackageConfig.exclude, true)
  	.then(exclude => {
  	const params = { exclude, include: this.getIncludes(funcPackageConfig.include) };

Then this.getIncludes and this.getExcludes resolve internal include and exclude patterns collections (which are not package config objects) which are processed further.

Technically to implement this functionality, all we need is to tweak this.getIncludes so it combines package.patterns into returned include patterns, and otherwise nothing else is needed to be done I think.

Of course it'll be also nice to clear internals so they also process single patterns collections and not both include and exclude, but as it can be seen a separate concern, and it's best if it's addressed with other PR (ideally PR's should be minimal and focus on one thing, especially that we deal here with really sensitive functionality).

Additionally we have a refactor planned to how we resolve files in a service and dev dependencies: #8586 and that will replace some of the logic in zipService and packageService, so it may not be worth to imply any refactors to it at this point

[medikoo]

Let's not change the way in which patterns are handled (this if needed should be handled in separate PR)

[juanjoDiaz]

I can do a separate commit or a separate PR if you'd rather that.
I've put micromatch back as it was and just added a comment.

[medikoo]

I can do a separate commit or a separate PR if you'd rather that.

Yes, that'll be great, and ideally if such change is first discussed, so before jumping into implementation we have full agreement that given refactor is justified. Please check #8586 it's probably a good point for discussing this

[medikoo]

Let's not update existing tests - technically they should remain passing without any changes.

I've proposed a refactor of them so they do not depend on internals: #8594, and I think we should address that first (so we don't fight with failing tests)

[juanjoDiaz]

With my latest changes, there are no changes in these tests anymore.

[juanjoDiaz]

Ok, I think that I've tackled everything.

Things left are:

• Are you happy with my change to the excludeDevDependencies method? In short, instead of a function that appends, to the exclude setting, a pure function that returns the patterns to append.
• Are you happy with the current getPatterns function?

Once we agree on those, and in any other that I've missed, I'll update the docs.

[juanjoDiaz]

This is now 3 commits.

• Hyper-minimal change as you suggested
• Use patterns in the create plugin
• Use patterns in the docs

Let me know if I still miss anything.

All the internals are still working with include/exclude so I'd really suggest doing a separate PR once this is merged to consolidate the internals to use just patterns

[medikoo]

@juanjoDiaz It looks great! I've just suggested a more permissive adaptation (so we support all three forms together with deprecation notice).

Also we need some tests, and for that ideally if #8594 is addressed first. Having that it'll be very easy to add few tests that confirm on patterns support

[medikoo]

This is actually a component template which is handled by @servereless/components, and changes we do here do influence that package. So we should not change that template

[juanjoDiaz]

If @servereless/components uses serverless, this will work just as it used to.

This will need to change for the next major version update anyway.
I think that keeping it is creating future trouble.

Do you disagree?

[medikoo]

If @servereless/components uses serverless,

It' doesn't use serverless(package internals), it's a completely different CLI

[medikoo]

I think this pattern should stay as it was

[juanjoDiaz]

This is a readme example that use to do:

package:
  include:
    - src/*.js
    - dist/*.js
    - dist/*.js.map

but I think that's a better example to exclude sources if we add the compiled files with their source-maps from dist and exclude the original files from src.

Do you dissagree?

[medikoo]

@juanjoDiaz this example intentionally includes sources in a bundle (note the sentence: To use a sourcemap, ensure that your packaging directory includes the compiled source)

Weather it's good example, and whether it could be improved it's the matter that's out of the scope of this PR. We may eventually open another issue to discuss it.

[medikoo]

Misses !

[medikoo]

I think this should not be changed

[juanjoDiaz]

This is example in aliyun, azure, google, kubeless, openwhisk and tencent.
So I think that it makes sense to use the same structure in aws for consistency (I fixed all the docs comparing them side by side).

Do you disagree?

[medikoo]

I wouldn't change in scope of this PR (it's not a matter we're addressing here). Also I think AWS version is better.

Also this documentation is scheduled to be secluded to general section and removed from providers, where it's duplicated in multiple places. Check #8085 & #8097

[medikoo]

Let's not update this header

[medikoo]

This seems not addressed

[medikoo]

Not addressed

[medikoo]

Should not be added

[medikoo]

Misses !

[medikoo]

I believe no exclamations marks should be added here, but to below patterns

[medikoo]

Misses !

[medikoo]

Misses !

[medikoo]

I think we need to support both include and patterns, in a way I proposed in this comment: #8581 (comment)

Technically we stand between choice:

1. If user provided patterns, throw if any include and exclude configurations are found
2. Support all three

I think it's easier (and not harmful) to support all three

[juanjoDiaz]

You also suggested to crash if both are used in comment: #8093 (comment) 😅

The problem of supporting both include and patterns is that the order in which they are appended becomes non-evident to the user and can cause tickets simply because the user expects a different order.

The current implementation support using include/exclude at the service level and patterns at the function level or viceversa. It errors if you use include/exclude and patterns in the same package object (done during the initialization validation).

I think that it's safer to avoid user confusion.

Do you still think that we should allow using both at the same time?

[medikoo]

You also suggested to crash if both are used in comment: #8093 (comment) 😅

That's true. Sorry for confusion. At that point it was not deeply thought. Thing is that crashing is more obtrusive.

Firstly the easy approach to this is to upfront investigate all package configs (service-level, function-level, layer-level) and crash if we find that there's patterns somewhere and includes or excludes elsewhere`, and that's aggressive if e.g. user is switching gradually, e.g. refactor first function one, check how it works and then refactor others. It'll be nice to not break that.

Now trying to validate case by case, is confusing, as e.g. for function 1 config (which uses just include and exclude) may be valid and for function 2 (which uses just patterns) may be not (when on service level include or exclude is used)

I assume it's not harmful to support all (user will always be approached with deprecation notice anyway), and implementation wise it's simplest approach.

The problem of supporting both include and patterns is that the order in which they are appended becomes non-evident to the user and can cause tickets simply because the user expects a different order.

User should not reason about order in such case, but simply ensure that just patterns is used , also again deprecation notice will signal that config stands on quirky ground

[juanjoDiaz]

Now trying to validate case by case, is confusing, as e.g. for function 1 config (which uses just include and exclude) may be valid and for function 2 (which uses just patterns) may be not (when on service level include or exclude is used)

That's not how it's implemented in this final version of the implementation. Function 1 and 2 will be valid in that example.

Function 3 which uses patterns and exclude under the same package object would be invalid.
I.e, to get the error you need something like:

package:
  patterns:
    - my-file.js
  include:
    - my-other-file.js

I think that's reasonable. However, if you still disagree, let me know and I'll just remove it. I'm running out of the time I've allocated for this 😅

[medikoo]

Function 1 and 2 will be valid in that example.

I think if we invalidate it on function level, we should definitely also not support combining include and exclude on service level with patterns on function level. That would be even more confusing (what's the logical reason for rejecting just one case?)

Anyway have you read the arguments I've put for supporting all without throwing (?) I take it's simplest thing we can do, and really not harmful

[juanjoDiaz]

I'm not sure if we are understanding each other completely or getting a bit tangled up. 😅
The validation for service level is exactly the same at the function level.
Let's give it one more go and then do whatever change it needed 😄

Currently:

• You can use patterns OR include/exclude at the service level.
• You can use patterns OR include/exclude at the function level.
• It doesn't matter if you use pattern in patterns at service and include/exclude at the function
• It doesn't matter if you use pattern in include/exclude at service and patterns at the function
• etc...

The only thing that fails, is to mix both patterns and include/exclude in the same config (regardless of whether it is in the service or function level).

Valid:

package:
  patterns:
    - my-file.js

functions:
  myFunction:
    package:
      include:
        - my-other-file.js
      exclude:
        - my-exclude-file.js

Valid:

package:
  include:
    - my-other-file.js
  exclude:
    - my-exclude-file.js

functions:
  myFunction:
    patterns:
      - my-file.js

Valid:

package:
  include:
    - my-other-file.js
  exclude:
    - my-exclude-file.js

functions:
  myFunction:
    patterns:
      - my-file.js
  myOtherFunction:
    package:
      include:
        - my-other-file.js
      exclude:
        - my-exclude-file.js

Throws:

package:
  patterns:
    - my-file.js
  include:
    - my-other-file.js
  exclude:
    - my-exclude-file.js

functions:
  myFunction:
    package:
      include:
        - my-other-file.js
      exclude:
        - my-exclude-file.js

Throws:

package:
  patterns:
    - my-file.js

functions:
  myFunction:
    package:
      patterns:
        - my-file.js
      include:
        - my-other-file.js
      exclude:
        - my-exclude-file.js

Just repeating myself, the reason to not allow that merging is that service goes before package regardless of whether it's patterns or include/exclude. And exclude goes before include.
However, if we mix include/exclude/pattern in the same config. We need to decide in which order to merge them.
It's trivial, I know, exclude -> include -> patterns, but users might expect otherwise and create confusion.

User should not reason about order in such case, but simply ensure that just patterns is used , also again deprecation notice will signal that config stands on quirky ground

That's right! That's why I thought that all combinations should be allowed, except for the one that combines both in the same object.

Was this already clear to you?

[medikoo]

You can use patterns OR include/exclude at the function level.
It doesn't matter if you use pattern in patterns at service and include/exclude at the function

In last comment, my point was that above feels confusing. If we already support mixing both include and patterns (coming from two configuration sources) to package given lambda, why restrict mixing of include and patterns coming from one configuration source.

However, if we mix include/exclude/pattern in the same config. We need to decide in which order to merge them.

Yes, I've proposed order in one of the first comments here: #8581 (comment)

It's trivial, I know, exclude -> include -> patterns, but users might expect otherwise and create confusion.

User should not investigate and debate on such order, instead should just use pattern. We'd allow mixing both sas temporary mean simply for ease of migration (no need to be too restrictive here)

[juanjoDiaz]

Alright, error removed.
Mixing is now allowed

[medikoo]

@juanjoDiaz is this ready for re-review? If it's the case please re-request it (in reviewers box). Thank you!

[juanjoDiaz]

Could we get this merged asap?
I keep rebasing but we keep getting merge conflicts because of other PRs taking precedence.

[medikoo]

Could we get this merged asap?
I keep rebasing but we keep getting merge conflicts because of other PRs taking precedence.

@juanjoDiaz Thanks for update. Sure, we'll look into that shortly. Note that it was stale for a long while, as review was not re-requested. I found that it has some new pushes, simply during my maintenance walk through over open PR's.

[medikoo]

@juanjoDiaz Looks great to me!

@pgrzesik I've reviewed this PR back in December, and now just re-reviewed changes after merge update. It all looks good to me, but can you re-review this PR with a fresh eye (please merge if there are no doubts)

[pgrzesik]

Thanks @juanjoDiaz 🙇 I have only two small things - one about adding deprecation code to deprecations.md section that I've mentioned in other comment. Another thing is that I'd like you to ask to update serverless.yml.md reference file: https://github.com/serverless/serverless/blob/master/docs/providers/aws/guide/serverless.yml.md so it reflects changes introduced in this PR. After having these, we're good to go 💯

[medikoo]

@juanjoDiaz do you plan to finalize this PR? If you lack resources we totally understand and we will happily take over, just let us know

[juanjoDiaz]

Hi @medikoo ,

Can you clarified what's left?

I've been attending all of your requests AFAIK. But then it goes stale and conflicts start arising as master moves forward.
I just rebased on top of master and this should be good to go unless there is anything else.

[medikoo]

Can you clarified what's left?

CI status was red, and we didn't have any notification that's ready for review.

When it's ready for review, please re-request it in top right box. Thank you!

[juanjoDiaz]

Alright!
CI is green and I've re-requested review to both reviewers.

[pgrzesik]

Thank you @juanjoDiaz 🙇 Looks good - @medikoo, could you also take a look at it before merging?

[medikoo]

Looks great! Thank you @juanjoDiaz

[tachyon83]

@juanjoDiaz @medikoo
I am very sorry to bother you guys. I am having a problem including some files in my serverless deployed function.

I have tried two ways:

1. package.exclude everything then include what I need (test files in src/test)
2. package.patterns to include what I need

But both are not working. I am just guessing the features you guys developed here are not applicable when using serverless-bundle plugin?

service: #####

plugins:
  - serverless-dotenv-plugin
  - serverless-bundle
  - serverless-offline

provider:
  name: aws
  runtime: nodejs14.x
  stage: ${opt:stage, 'local'}
  memorySize: 1024
  timeout: 25
  # some private properties are omitted

package:
  individually: true

custom:
  sls:
    version: v1
  serverless-offline:
    httpPort: #####
    lambdaPort: #####
    noPrependStageInUrl: true
    allowCache: true
  dotenv:
    logging: true
  bundle:
    sourcemaps: false
    externals: all

functions:
  api:
    handler: src/main.handler
    name: main-function
    events:
      - httpApi:
          method: any
          path: /
      - httpApi:
          method: any
          path: /{proxy+}

  test:
    handler: src/e2eTest.handler
    name: e2e-test-function
    timeout: 900
    package:
      exclude:
        - ./**
      include:
        - "src/test/**"

[medikoo]

But both are not working. I am just guessing the features you guys developed here are not applicable when using serverless-bundle plugin?

It could be the case. Best is to confirm with plugin directly (and I see you've opened AnomalyInnovations/serverless-bundle#363)

[juanjoDiaz]

A quick search of serverless-bundle codebase shows that they have not updated to patterns yet.