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.

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

Add print command to generate output of computed serverless.yml file with all resolved Serverless Variables #4169

Merged
merged 18 commits into from Oct 14, 2017

Conversation

@alexdebrie
Copy link
Contributor

commented Aug 29, 2017

What did you implement:

Adds an sls print command that compiles your config file, resolves all variable references, and prints it to the console.

Closes #3224

Would also assist in debugging reported issues such as #4161.

How did you implement it:

Use the getServerlessConfigFile() utility function to read the config file, then use the populateObject() method to resolve all references.

How can we verify it:

Edited August 31 to use CLI options

Create a Serverless service with the following files:

# serverless.yml

service: print-test

custom:
  region: ${opt:region}
  bucketName: mySpecialBucket
  fileValue: ${file(./config.yml):secretValue}

provider:
  name: aws
  runtime: python3.6

package:
  include:
    - include-me.py
    - include-me-dir/**

functions:
  hello:
    handler: handler.hello

resources:
  Resources:
    NewResource:
      Type: AWS::S3::Bucket
      Properties:
        BucketName: ${self:custom.bucketName}
#config.yml

secretValue: ohsosecret

Then check that it works:

$ sls print --region us-east-1
service: lint-test
frameworkVersion: =X.X.X
custom:
  region: us-east-1
  bucketName: mySpecialBucket
  fileValue: ohsosecret <-- Resolved!!
provider:
  name: aws
  runtime: python3.6
package:
  include:
    - include-me.py
    - include-me-dir/**
functions:
  hello:
    handler: handler.hello
resources:
  Resources:
    NewResource:
      Type: 'AWS::S3::Bucket'
      Properties:
        BucketName: mySpecialBucket <--- Resolved!!

Todos:

  • Write tests
  • Write documentation
  • Fix linting errors
  • Make sure code coverage hasn't dropped
  • Provide verification config / commands / resources
  • Enable "Allow edits from maintainers" for this PR
  • Update the messages below

Is this ready for review?: YES
Is it a breaking change?: NO

@pmuens pmuens self-requested a review Aug 29, 2017
@pmuens pmuens changed the title Add print command to display config Add print command to generate output of computed serverless.yml file with all resolved Serverless Variables Aug 31, 2017
@eahefnawy

This comment has been minimized.

Copy link
Member

commented Aug 31, 2017

Thanks for this PR @alexdebrie 😊 ... I wish it was that simple though! :D ... options references complicates things a little. So If you're referencing options, you need to make sure you provide it to the print command as well.

So, what you'll need to do is use populateService and pass it the options object instead of using populateObject.

@eahefnawy eahefnawy referenced this pull request Aug 31, 2017
@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Aug 31, 2017

Thanks for that note @eahefnawy! I rarely use CLI options so didn't think to test it 😁.

Can you check the latest commit? I just set the options property in the variables class before running populateObject. I don't love populateService because it returns a ton of other information in the return object.

Copy link
Member

left a comment

I can't tell from the code, maybe it already does this, but similarly to -o it should also support -f to support per-function env vars.

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@dschep I'm not understanding there, can you elaborate? What would you be passing in on the command line with -f?

If you're asking if it would resolve variables in the function environment variables section, it would. It resolves the entire config file.

@dschep

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Doh, I misunderstood what this outputs, this outputs serverless.yml with vars resolved, not the environment with config variables resolved.

ignoreme

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

Alright! sounds good @alexdebrie 🙌 I've tried it out with the extensive variable config mentioned in #4144 and it's working with all cases 👏

So in terms of functionality, this is good to go. @alexdebrie could you add tests and docs for this new command 😊 ... then it should be G2M!

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Sep 1, 2017

@eahefnawy Will do!

Also, do you know why CI is failing? For older Node versions, it's having trouble with the Plugins.json file. Not sure why.

@eahefnawy eahefnawy referenced this pull request Sep 1, 2017
7 of 7 tasks complete
@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@alexdebrie I don't think this is the real issue. I've seen this error many times masking the real cause of the error. I think it's an issue in our codebase where this error swallows the actual error. We need to fix that.

I was able to reproduce this error after switching to Node 4, I'll try to debug the real issue here with you and let you know what's going on if I find something

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@alexdebrie I figured it out. You need to add 'use strict'; at the beginning of the file 😅 ... freakin' Node!

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

I just added it and pushed it to the branch

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

It's interesting to note that this little issue in the print plugin broke all plugins in Node 4/5 ... This is a fundamental issue in our plugin system where we actually load the logic of all the plugins even when we're using only one plugin. So a syntax error or something in one plugin would break the entire framework. Even more bad that it's masked by this misleading error.

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Sep 1, 2017

@eahefnawy I'm not sure if the plugin loading algorithm is really an issue. On the other hand it shows errors immediately. If you'd only load a few of them (the needed ones), you might release without even noticing that something's wrong.

Additionally the forgotten 'use strict' is a major issue in my opinion, because it can f*** up the whole app. The only way to improve the quality checks that I see is, to run all unit tests with Node 4, because Node 6+ won't show them.

@mvayngrib

This comment has been minimized.

Copy link
Contributor

commented Sep 3, 2017

@alexdebrie i've wanted this one for a while!

it seems to choke if a different variableSyntax is specified, e.g.

  variableSyntax: "\\${{([\\s\\S]+?)}}"
@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 4, 2017

@mvayngrib thank you! really nice catch! @alexdebrie that's another reason to use populateService 😊

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Sep 4, 2017

Thanks @mvayngrib! I'll get that fixed.

@eahefnawy I'd like to stay away from populateService because it adds a bunch of other crud that's not directly relevant here -- I'd like to have a easy, clear way to understand what my config files. I'll need to do some additional looking on this, but it seems like there should be a pure function that says "Here's my config file, return the fully resolved config object" without relying on a bunch of separate state.

@eahefnawy

This comment has been minimized.

Copy link
Member

commented Sep 5, 2017

@alexdebrie sounds good! however you see fit. We def need pure functions! Converting our codebase to be more functional is a big task that I wanted to do for so long already!

@pmuens

This comment has been minimized.

Copy link
Member

commented Sep 20, 2017

@alexdebrie Any updates on this one?

Let us know if you need any help here! I really like this feature and it would be great if we could introduce that in one of the upcoming versions 🎉 👍.

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Sep 20, 2017

@pmuens Thanks for the reminder. I got sucked into a rabbit hole trying to make it perfect and then put it on the backburner.

Working on this today.

@mvayngrib

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@dschep whoa, are those tricks with &, * and << documented somewhere?

@dschep

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

@mvayngrib yup. The YAML spec 😉 http://yaml.org/type/merge.html

edit: I think that's only part of it (the <<)

@mvayngrib

This comment has been minimized.

Copy link
Contributor

commented Oct 4, 2017

@dschep haha, thanks! If you remember where u saw the others, let me know

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

You can find the other ones in http://www.yaml.org/spec/1.2/spec.html

Anchors (&) and aliases (*)
6.9.2 Node Anchors
7.1. Alias Nodes

BTW: I assume our YAML parser also supports the tons of other nifty stuff in there 😄

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Oct 4, 2017

Nice catch, @dschep! I just added a new option for dump() that should handle this.

@dschep

This comment has been minimized.

Copy link
Member

commented Oct 4, 2017

Awesome @alexdebrie, works as expected now! 😄 🎉

@RafalWilinski

This comment has been minimized.

Copy link
Contributor

commented Oct 11, 2017

Thanks @alexdebrie for contribution! 💪

I confirm that it works as expected. Besides a small eslint bug that I've just fixed, everything seems great from my perspective.

LGTM

Merging as soon as CI pass...

@horike37

This comment has been minimized.

Copy link
Member

commented Oct 12, 2017

Hey, @alexdebrie 👍 Thank you for the great PR 💯
Could you please rebase this PR since the error on CI has been resolved on the top of master?
Thanks in advance!

@horike37 horike37 added this to the 1.24 milestone Oct 12, 2017
Copy link
Member

left a comment

Hey @alexdebrie, nevermind my above comment.
Rebasing has been done on my end 👍

I just tested locally and worked fine for me!
That's an awesome functionality, so I can't wait for the upcoming release 💯

While working on this, webtask provider has been added. Therefore you might want to add this functionality to the webtask doc as well.

https://travis-ci.org/serverless/serverless/jobs/287308556
The integration test has been failed. Something might go wrong on this PR.. Could you take a look into it?

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Oct 14, 2017

@RafalWilinski Thanks for fixing the eslint bug.

@horike37 Thanks for rebasing and for the other comments. It looks like the integration tests were just flaky, as I reran them and they have passed.

Re: Webtasks, I only added documentation for providers that have a Variables section in their documentation with actual variables examples. Some of the providers are more straight-forward in their config and don't use much resolved variables.

Good to merge on my end 😄

@horike37

This comment has been minimized.

Copy link
Member

commented Oct 14, 2017

Thank you for commenting @alexdebrie 👍
LGTM 💯 merging...

@horike37 horike37 merged commit 97d80b3 into master Oct 14, 2017
5 checks passed
5 checks passed
continuous-integration/appveyor/branch AppVeyor build succeeded
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.03%) to 87.417%
Details
@horike37 horike37 deleted the SLSPrint branch Oct 14, 2017
@benswinburne

This comment has been minimized.

Copy link

commented Oct 18, 2017

What version of Serverless will this be published in? It's in the 1.23.0 docs here but doesn't appear to be in Serverless 1.23.0 (which makes sense as that was published to npm 4 weeks ago and this was 4 days ago)

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Oct 18, 2017

@benswinburne This will be in the 1.24 release. If you'd like this feature now, you can install the master branch of this repo with:

npm install -g serverless/serverless
@benswinburne

This comment has been minimized.

Copy link

commented Oct 18, 2017

I've checked out 97d80b3 to play around with it as it stands. Don't suppose you know when 1.24 is likely/planned to release do?

@ozbillwang

This comment has been minimized.

Copy link

commented Nov 1, 2017

@alexdebrie

I have tested with latest sls release 1.24, print works very well.

Thanks a lot to add this feature in sls.

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

Great to hear, @ozbillwang ! I'd love to do a linter with this eventually, but it's on my backlog 😄

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

@alexdebrie @horike37 @RafalWilinski Did we ever think about creating an ESLint plugin for serverless, that will analyze the serverless.yml and report errors in there in real-time? Just had that idea and it should eliminate lots of issues that exist due to configuration errors. It might be a bit complex, but in the end very helpful. The checks could include variables, structure, function definitions, etc.
@pmuens If we ever decide to create such a plugin, it would be good to have it hosted as serverless repo.

Any comments?

@alexdebrie

This comment has been minimized.

Copy link
Contributor Author

commented Nov 2, 2017

@HyperBrain I definitely like the idea, particularly because configuration issues are one of the big things I see over and over.

There could be a few difficulties:

  1. If it were separate from the serverless core, it would have to do a lot of work to emulate the Serverless variable system. If the serverless.yml has something like ${self:custom.foo.${opt:stage}}, it has to know how to resolve all of that.

  2. The config often relies on command like options like --stage, so we'd need to think about sane defaults for the linter.

  3. Would need to think about how to handle variables that take a while to resolve (S3, SSM, other CloudFormation stacks, JS files). This could also cause rate-limit issues with CloudFormation / SSM.

For these reasons, I was thinking a good first step would be to write a linter where you could do sls print --stage prod | sls-lint. This would handle the three problems above, with the downside that you need to manually kick off a compile + lint step.

@HyperBrain

This comment has been minimized.

Copy link
Member

commented Nov 2, 2017

I agree with the on-demand first step approach. That makes sense.

For an ESLint plugin I would go for pure syntax checking in the first step. That means that it checks for correct root nodes in the serverless.yml, variable reference syntax, missing mandatory properties on nodes, deprecated definitions, etc.

The config often relies on command like options like --stage, so we'd need to think about sane defaults for the linter.
Would need to think about how to handle variables that take a while to resolve (S3, SSM, other CloudFormation stacks, JS files). This could also cause rate-limit issues with CloudFormation / SSM.

For these semantical checks I'm not really sure if an ESLint plugin is the right place to do that, because most likely it is run within an editor and any external calls (especially to AWS) are rate limited and slow. Of course a proper request queuing can prevent issues there.
So I see these as a further evolution - if not completely unnecessary and replaced by your approach.

@RafalWilinski

This comment has been minimized.

Copy link
Contributor

commented Nov 2, 2017

Great idea @HyperBrain, I make a lot of mistakes when writing serverless.yml file so such thing would be really useful. Actually, in past I thought about adding a "Language Support" for VS Code or Atom so we could have IntelliSense autosuggestions (e.g. when inside function object, IDE suggests adding handler, events etc.) in realtime when typing but that's quite complex.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
10 participants
You can’t perform that action at this time.