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

feat: Add !merge YAML tag support #79

Closed
wants to merge 1 commit into from
Closed

feat: Add !merge YAML tag support #79

wants to merge 1 commit into from

Conversation

glb
Copy link

@glb glb commented Apr 16, 2021

This extends the YAML schema to support a !merge tag as described in serverless/serverless#8037:

Example: merge objects

provider:
  environment: !merge
    - FOO: value1
    - BAR: value2

results in:

provider:
  environment:
    FOO: value1
    BAR: value2

which may seem less-than-thrilling but if you consider the case of

provider:
  environment: !merge
    - ${file(stage-env.json)}
    - ${file(common-env.json)}

or:

resources:
  Resources: !merge
    - ${file(resources1.yaml)}
    - ${file(resources2.yaml)}

it becomes more interesting.

🚨 This will throw an error if there are key collisions in the list of objects given to !merge; it will not simply overwrite the keys like what happens with Object.assign({}, ...).

Example: "merge" arrays

If !merge is given a list of arrays, it will do a shallow flattening of the provided arrays and return a list that is the equivalent of concatenating all of the lists together. There is no detection or removal of duplicate elements.

resources:
  Resources:
    example:
      Type: AWS::S3::Bucket
      DependsOn: !merge
        - ["a", "b"]
        - ["c", "d"]

becomes

resources:
  Resources:
    example:
      Type: AWS::S3::Bucket
      DependsOn:
        - "a"
        - "b"
        - "c"
        - "d"

which again is more interesting if you consider the opportunities for merging more complex content.

@codecov
Copy link

codecov bot commented Apr 16, 2021

Codecov Report

❗ No coverage uploaded for pull request base (master@adbabd1). Click here to learn what that means.
The diff coverage is 100.00%.

Impacted file tree graph

@@            Coverage Diff            @@
##             master      #79   +/-   ##
=========================================
  Coverage          ?   91.59%           
=========================================
  Files             ?        9           
  Lines             ?      226           
  Branches          ?        0           
=========================================
  Hits              ?      207           
  Misses            ?       19           
  Partials          ?        0           
Impacted Files Coverage Δ
cloudformation-schema.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 adbabd1...0285791. Read the comment docs.

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@glb great thanks for this! Still can you follow the specification and propose it in https://github.com/serverless/serverless/ repository?

If you have some concerns towards specification or something is not clear, please ask at serverless/serverless#8037

@glb
Copy link
Author

glb commented Apr 19, 2021

@glb great thanks for this! Still can you follow the specification and propose it in https://github.com/serverless/serverless/ repository?

If you have some concerns towards specification or something is not clear, please ask at serverless/serverless#8037

@medikoo could you please help me understand where I've diverged from the specification in serverless/serverless#8037 in this PR? This is the change for part 2.

I'm actually a little unsure whether this will work as desired; I am trying to do an integration test of @serverless/serverless and this @serverless/utils branch with a serverless.yml file that has

provider:
  environment: !merge
    - ${file(stage-env.json)}
    - ${file(common-env.json)}

but running into what I think is an unrelated problem.

I'm not at all sure whether the !merge tag processing will work as desired, as I think it happens during YAML parsing and before variable resolution...

@medikoo
Copy link
Contributor

medikoo commented Apr 19, 2021

@medikoo could you please help me understand where I've diverged from the specification in serverless/serverless#8037 in this PR? This is the change for part 2.

Specification mentions:

Let's put definition of new YAML types to lib/configuration/yaml-merge.js

And that's in context of serverless/serverless repository.

We do not expect any PR to be made in this repository

@glb glb closed this Apr 19, 2021
@glb glb deleted the 8037-2-merge-tag-support branch April 19, 2021 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants