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

Append in Custom Syntax #5799

Merged
merged 1 commit into from Feb 6, 2019

Conversation

Projects
None yet
3 participants
@erikerikson
Copy link
Member

erikerikson commented Feb 6, 2019

What did you implement:

Respect custom variable syntaxes when appending to deep variables.

Closes #5769

How did you implement it:

How can we verify it:

See new unit test. For manual confirmation, run sls print against the following assets from #5769:

env.yml

description: this is dummy

serverless.yml - Errorneous template with a custom variable syntax


provider:
  name: aws
  runtime: nodejs8.10
  variableSyntax: "\\${{([ ~:a-zA-Z0-9._@\\'\",\\-\\/\\(\\)]+?)}}"

custom:
  settings: ${{file(./env.yml)}}

functions:
  hello:
    handler: handler.hello
    description: ${{self:custom.settings.description}}

Todos:

  • Write tests
  • [n/a] 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

Append in Custom Syntax
Respect custom variable syntaxes when appending to deep variables.
@pmuens

pmuens approved these changes Feb 6, 2019

Copy link
Member

pmuens left a comment

Great stuff! Just tried it and it works fine 👍

Great job @erikerikson 💯

I haven't tested other variables usages thoroughly. Is there a huge likelyhood that this could break existing variables usages? The surface area of this change seems quite small and our Serverless Varaibles test coverage looks decent enough. Just asking since you and @eahefnawy are the Serverless Variables experts 😅

If that's not the case I'd say it should be GTM :shipit: 👌

@erikerikson

This comment has been minimized.

Copy link
Member Author

erikerikson commented Feb 6, 2019

This is much more of a unhandled corner case that is now properly covered. 😄. I'd call the likelihood of breaking existing variable usage incredibly low but would be happy to have second opinions.

One of the big efforts I've made in the variables as part of my contribution is a large expansion of user level variable behavior specifications via tests. To be honest, mostly because I lost trust in my ability to reason over the effects of changes in the test could be complete and thorough, so every time there's a bug I've added coverage to make sure it doesn't crop back up. You can see the section around where I've added the test in this PR to see what I mean.

@dschep dschep merged commit d53081a into serverless:master Feb 6, 2019

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@pmuens pmuens added this to the 1.38.0 milestone Feb 7, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.