feat(Variables): Introduce unresolvedVariablesNotificationMode by G-Rath · Pull Request #8710 · serverless/serverless

G-Rath · 2021-01-04T19:40:39Z

I've followed the implementation suggested in the original issue, with some minor modification due to the issues age that I think meant the implementation got a little out of date.

In particular, I've changed the option to be a boolean rather than an enum as I think that's less ambiguous and everything on the issue imo points to this not being a mode like configValidationMode. It's also an easy enough change to switch it back to an enum if that is desired.

Because warnIfNotFound isn't marked as private I took the safe approach of not modifying it's behaviour aside from having it print the deprecation, or renaming it - this way if it's used by any plugins etc it shouldn't be breaking.

Test-wise I've just copied the existing tests for warnIfNotFound and refactored them to check to.throw & not.to.throw. I've duplicated the tests for checking the behaviour of throwOnUnresolvedVariableReferences when it's false rather than try to be clever by somehow doing an expect(warnIfNotFound).to.have.been.called() on the test subject.

Overall I've gone for boring and simple rather than fancy and completely DRY.

Closes: #3367

codecov · 2021-01-04T19:43:06Z

Codecov Report

Merging #8710 (29d7fd6) into master (b4fef7d) will increase coverage by 0.08%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master    #8710      +/-   ##
==========================================
+ Coverage   87.43%   87.52%   +0.08%     
==========================================
  Files         256      259       +3     
  Lines        9631     9635       +4     
==========================================
+ Hits         8421     8433      +12     
+ Misses       1210     1202       -8

Impacted Files	Coverage Δ
lib/configSchema.js	`100.00% <ø> (ø)`
lib/classes/Service.js	`85.60% <100.00%> (+0.11%)`	⬆️
lib/classes/Variables.js	`99.73% <100.00%> (+<0.01%)`	⬆️
lib/plugins/aws/deploy/index.js	`96.55% <0.00%> (-1.73%)`	⬇️
lib/classes/Error.js	`95.45% <0.00%> (-0.55%)`	⬇️
lib/classes/CLI.js	`92.81% <0.00%> (-0.22%)`	⬇️
lib/plugins/aws/invokeLocal/index.js	`67.50% <0.00%> (-0.19%)`	⬇️
lib/Serverless.js	`97.16% <0.00%> (-0.03%)`	⬇️
lib/plugins/aws/customResources/generateZip.js	`100.00% <0.00%> (ø)`
lib/plugins/aws/package/lib/mergeIamTemplates.js	`100.00% <0.00%> (ø)`
... and 7 more

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 b4fef7d...29d7fd6. Read the comment docs.

pgrzesik

Thank you @G-Rath, it looks good overall. Please see my comments, I'll also request an extra review from @medikoo as he's more familiar with the original issue.

pgrzesik · 2021-01-05T10:32:08Z

 frameworkVersion: '2'
 enableLocalInstallationFallback: false # If set to 'true', guarantees that it's a locally (for service, in its node_modules) installed framework which processes the command
 useDotenv: false # If set to 'true', environment variables will be automatically loaded from .env files
+throwOnUnresolvedVariableReferences: false # If set to 'true', references to variables that cannot be resolved will result in an error being thrown


I think I would prefer sticking to unresolvedVariablesNotificationMode as enum as that gives us a bit more flexibility in the future if needed (if there would be a request to introduce another mode, or e.g. turning warnings/errors completely)

pgrzesik · 2021-01-05T10:41:07Z

      expect(logWarningSpy.args[0][0]).to.contain('file');
    });
  });
+  describe('#assertFound()', () => {


I think it would be better to try to use runServerless-based tests to test out the newly introduced funtionality here. We try to migrate all tests to runServerless, here you can see an example on how that might look like: https://github.com/G-Rath/serverless/blob/master/test/unit/lib/classes/Variables.test.js#L2745-L2783

Sure, I'll give it a try - however, if I understand things correctly that'll result in a lot of fixtures (and possible high slowdown since those tests are expensive?) since we error on the first unresolved variable.

So if we want to have the same test coverage as before we'd need to have a test/fixture for each possible value of varType, for each possible mode supported by unresolvedVariablesNotificationMode.

A possible way around this could be keep the _describeVariableType method and unit test that, and use runServerless tests for the rest (which'd mean only needing 2 or 3 tests)

Sure, I'll give it a try - however, if I understand things correctly that'll result in a lot of fixtures

We should reuse existing fixture (variables), which we can simply extend for every case

(and possible high slowdown since those tests are expensive?)

They're somewhat expensive, and it's good to minimize runServerless runs when possible, but all error cases are test ed individually, and in this case we need to do different runServerless run per fixture.

Test best if are added at the bottom of tests files, where we already have some runServerless based tests added

So I've been trying to write a single test for now, but no matter what I do unresolvedVariablesNotificationMode doesn't seem to be making it into the config - is there anything I might have missed?

@G-Rath can you commit the code, that you feel that should work and doesn't work for you?

Technically we're not meant to test print out in this variables.test.js

But when adding new property ideally it should be onfirmed that it works as intended in all areas, otherwise submission is not really complete (and we avoid introducing incomplete/WIP merges to master)

Technically we're not meant to test print out in this variables.test.js

I think the bug is elsewhere, as it looks like variables have already been resolved by the time print is being called (e.g. custom.myVariable is undefined and the warning has been printed) - tbh it could be the test runner is just truncating the output.

Seems that the property is available on service (I'm guessing because I'm assigning it in Service.js) - does that sound like a valid way to access it, or should I continue to try and get it via serverless.config?

Seems that the property is available on service

On service instance? Or on service config property (which is accessible at service.service) ?

Yes, on the this.service instance.

I've got things working now, and so have written tests + cleaned up my commit - should be ready for re-review :)

medikoo

Thank you @G-Rath for this improvement!

I agree with all points raised by @pgrzesik and added few more comments.

Let us know

medikoo · 2021-01-05T16:52:22Z

+    return null;
+  }
+
+  assertFound(variableString, valueToPopulate) {


Why do we need two methods? Can't we integrate this handling in existing warnIfNotFound (which we can rename into e.g. handleUnresolved)

I wasn't sure if the current method was being used by any plugins, and so wanted to keep the breaking change as small as possible.

Happy to do it as one method if you're fine with that - ~~I went with assertFound for the name as that follows the name for methods in the assert module and so indicates that the method is an assertion guard.~~

(handleUnresolved makes more sense as a name if unresolvedVariablesNotificationMode is an enum)

I wasn't sure if the current method was being used by any plugins

I think it's very unlikely, also it's considered as private part, so plugins should really not do that

Awesome, I'll merge them into a single method then 🎉

Do you want me to do any form of underscore naming to highlight their private? I've noticed that's done in a couple of other places in the framework :)

Do you want me to do any form of underscore naming to highlight their private? I've noticed that's done in a couple of other places in the framework :)

I believe it's only done in places where we want to distinct private API from public. Here it's not the case (whole class is private)

Fair enough

pgrzesik

Looks good to me, well done @G-Rath. Lets wait for @medikoo approval before merging

medikoo

Thank you @G-Rath ! It looks very good! I have just few minor final suggestions

medikoo · 2021-01-07T12:31:30Z

+        const out = await runServerless({
+          fixture: 'variables',
+          cliArgs: ['print'],
+          configExt: {
+            unresolvedVariablesNotificationMode: 'warn',
+            custom: {
+              myVariable1: '${env:missingEnvVar}',
+              myVariable2: '${opt:missingOpt}',
+              myVariable3: '${self:missingAttribute}',
+              myVariable4: '${file(./missingFile)}',
+            },
+          },
+        });
+
+        ({ serverless } = out);


Why not:

const { serverless: { stdout } } = await runServerless(...);

Because serverless is declared here as a let, meaning this'll be a shadowing that eslint errors on.

But in this case you do not access serverless but just stdout (and it's only that which I define in snippet)

It's used on the line straight after this one - but I guess actually I could do:

const { serverless: { triggeredDeprecations }, stdoutData } = await runServerless(...);

I've actually didn't notice need to access serverless.triggeredDeprecations, so sorry for misleading suggestion

Anyway it was flawed that runServerless tests share same context as other tests, it should not be the case. I've fixed that in master, and now there's no serverless shadowing, please rebase.

medikoo

Thank you @G-Rath it looks really good!

I have just last few minor concerns and we should be good to go

medikoo

Thanks @G-Rath for update! It looks very good, I have just last question

medikoo

Thank you @G-Rath! I have just two final cleanup suggestions, and we're good to go

medikoo

Thank you @G-Rath it looks great to me!

I'm leaving final approval and merge to @pgrzesik

pgrzesik

Thanks @G-Rath for the awesome job and @medikoo for all the reviews - there's one last small nitpick I have but otherwise we're good to go 🎉

pgrzesik

Thank you @G-Rath, well done 🙌

tauil · 2021-11-25T12:26:21Z

I just would like to thank you all for that feature! ❤️

codecov-commenter · 2026-04-01T05:31:50Z

⚠️ Please install the to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.52%. Comparing base (b4fef7d) to head (29d7fd6).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #8710      +/-   ##
==========================================
+ Coverage   87.44%   87.52%   +0.07%     
==========================================
  Files         256      259       +3     
  Lines        9631     9635       +4     
==========================================
+ Hits         8422     8433      +11     
+ Misses       1209     1202       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

G-Rath mentioned this pull request Jan 4, 2021

Throw when env variables are missing #8700

Closed

pgrzesik suggested changes Jan 5, 2021

View reviewed changes

pgrzesik added cat/variable deprecation Deprecation proposal (breaking with next major) enhancement labels Jan 5, 2021

medikoo suggested changes Jan 5, 2021

View reviewed changes

medikoo assigned G-Rath Jan 5, 2021

G-Rath requested review from medikoo and pgrzesik January 6, 2021 19:59

pgrzesik previously approved these changes Jan 7, 2021

View reviewed changes

medikoo suggested changes Jan 7, 2021

View reviewed changes

G-Rath dismissed pgrzesik’s stale review via b053812 January 7, 2021 18:11

G-Rath requested review from medikoo and pgrzesik January 7, 2021 18:28

medikoo suggested changes Jan 8, 2021

View reviewed changes

Comment thread test/unit/lib/classes/Variables.test.js

G-Rath requested a review from medikoo January 8, 2021 17:50

G-Rath added 3 commits January 11, 2021 09:24

feat: throw when a variable reference cannot be resolved

cc2e30c

feat: replace bool option with enum and test using runServerless

30a4fc8

feat: use error code when throwing

31f088f

medikoo suggested changes Jan 11, 2021

View reviewed changes

Comment thread test/unit/lib/classes/Variables.test.js Outdated

test: add unit tests for handleUnresolved

b755ac9

G-Rath requested a review from medikoo January 11, 2021 19:53

medikoo suggested changes Jan 12, 2021

View reviewed changes

Comment thread lib/classes/Service.js Outdated

Comment thread lib/classes/Variables.js Outdated

chore: remove comments

3083e61

G-Rath requested a review from medikoo January 12, 2021 17:06

medikoo previously approved these changes Jan 13, 2021

View reviewed changes

pgrzesik suggested changes Jan 13, 2021

View reviewed changes

Comment thread docs/deprecations.md

docs: add deprecation code for VARIABLES_ERROR_ON_UNRESOLVED

29d7fd6

G-Rath dismissed medikoo’s stale review via 29d7fd6 January 14, 2021 03:02

G-Rath requested review from medikoo and pgrzesik January 14, 2021 03:05

pgrzesik approved these changes Jan 14, 2021

View reviewed changes

pgrzesik changed the title ~~feat: throw when a variable reference cannot be resolved~~ feat(Variables): Introduce unresolvedVariablesNotificationMode Jan 14, 2021

pgrzesik merged commit 33cffc3 into serverless:master Jan 14, 2021

G-Rath deleted the throw-when-variable-not-resolved branch January 17, 2021 17:52

G-Rath mentioned this pull request Feb 8, 2021

fix(serverless): update config properties DefinitelyTyped/DefinitelyTyped#51130

Merged

8 tasks

Uh oh!

Conversation

G-Rath commented Jan 4, 2021

Uh oh!

codecov Bot commented Jan 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pgrzesik left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G-Rath Jan 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G-Rath Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

G-Rath Jan 5, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pgrzesik left a comment

Choose a reason for hiding this comment

Uh oh!

medikoo left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

medikoo Jan 8, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

codecov Bot commented Jan 4, 2021 •

edited

Loading

G-Rath Jan 6, 2021 •

edited

Loading

G-Rath Jan 5, 2021 •

edited

Loading

G-Rath Jan 5, 2021 •

edited

Loading

medikoo Jan 8, 2021 •

edited

Loading