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

Fix #3184 Compatibility with native AWS CloudFormation variable syntax #8279

Merged
merged 3 commits into from Sep 24, 2020

Conversation

mnapoli
Copy link
Contributor

@mnapoli mnapoli commented Sep 23, 2020

Closes: #3184

This PR brings support for native CloudFormation variables, such as ${AWS::Region} or CloudFormation references (e.g. ${MyS3Bucket}). I haven't tested yet if !Sub works correctly after this change, maybe there are additional changes to do for that.

I applied the solution described in #3184 by @medikoo (change the variableSyntax regular expression), however I had to tune it a bit to preserve support for the ${file(...)} syntax.

I tested the regex with these patterns:

# ignored without errors:
${AWS::Region}
${Database}

# parsed by the serverless framework:
${foo:bar}
${self:custom.ini, "foo"}
${self:custom.ini, "fallback123_*"}
${self:custom.ini, "hello+world^*$@(!"}
${self:custom.ini, "+++++"}
${self:custom.ini, "システム管理者*"}
${file(~/config.yml)}
${file(~/somedir/config.yml)}
${file(../config.json):CREDS}
${self:}

I also had to add some tests to cover better some syntax. I'll add some inline comments.

readFileSyncStub.restore();
fileExistsStub.restore();
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I added a higher level test here because there was no test covering that ${file(...)} is actually parsed correctly. When I changed the regex, initially the tests passed but it was a false positive: that syntax was broken. So I added that test and iterated on the regex.

@codecov-commenter
Copy link

codecov-commenter commented Sep 23, 2020

Codecov Report

Merging #8279 into master will increase coverage by 0.07%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8279      +/-   ##
==========================================
+ Coverage   88.11%   88.19%   +0.07%     
==========================================
  Files         250      250              
  Lines        9365     9400      +35     
==========================================
+ Hits         8252     8290      +38     
+ Misses       1113     1110       -3     
Impacted Files Coverage Δ
lib/classes/Service.js 85.48% <ø> (ø)
lib/plugins/aws/package/compile/functions/index.js 96.64% <0.00%> (-0.40%) ⬇️
lib/plugins/aws/package/compile/layers/index.js 95.60% <0.00%> (ø)
lib/plugins/aws/provider/awsProvider.js 93.18% <0.00%> (+0.07%) ⬆️
lib/plugins/package/lib/packageService.js 90.16% <0.00%> (+4.09%) ⬆️

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 76e02cc...b26eeee. Read the comment docs.

expect(print.serverless.cli.consoleLog.called).to.be.equal(true);
expect(YAML.load(message)).to.eql(expected);
});
});
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I removed that test: it tested that the ${self:} syntax worked. I had a look at the git history and the rest of the project but couldn't understand why this was tested.

Either I missed something and we should bring that back + cover it with the new regex (let me know), or it's something that isn't necessary anymore and we can move forward.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good call. I would add testing some of ${self: ..} to `variables fixture as proposed in other comment

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.

@mnapoli thank you ! That looks great. See my proposal on (I believe) better approach to testing. Having that set up with runServerless, will make it really solid I believe

lib/classes/Service.js Outdated Show resolved Hide resolved
Comment on lines 1363 to 1382
it('should support ${file(...)} syntax', () => {
// eslint-disable-next-line no-template-curly-in-string
const property = '${file(~/somedir/../config.yml)}';

const expectedFileName = `${os.homedir()}/somedir/../config.yml`;
const configYml = {
foo: 'bar',
};
const fileExistsStub = sinon.stub(serverless.utils, 'fileExistsSync').returns(true);
const realpathSync = sinon.stub(fse, 'realpathSync').returns(expectedFileName);
const readFileSyncStub = sinon.stub(serverless.utils, 'readFileSync').returns(configYml);

return serverless.variables
.populateProperty(property)
.then(valueToPopulate => {
expect(realpathSync).to.not.have.been.called;
expect(fileExistsStub).to.have.been.calledWithMatch(expectedFileName);
expect(readFileSyncStub).to.have.been.calledWithMatch(expectedFileName);
expect(valueToPopulate).to.deep.equal(configYml);
})
.finally(() => {
realpathSync.restore();
readFileSyncStub.restore();
fileExistsStub.restore();
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks @mnapoli that's a very good call! Still we configure all new tests with runServerless (see https://github.com/serverless/serverless/tree/master/test#unit-tests) and I think this tests can also be way better expressed with that.

I propose to introduce a variables fixture, with simple config that references real file via ${file( variable, and other cases we want to test when covering this PR.

Then in tests we will just confirm that resolved config reflects content from imported file (no stubs needed)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Oh wow that new approach is a THOUSAND times better! To be honest every time I tried to contribute to the project I was struggling so much with the tests and the mocks. But this is awesome, I did struggle to get it working at first but now it feels much better, and safer.

I updated the PR, let me know if this is what you had in mind.

lib/classes/Variables.test.js Outdated Show resolved Hide resolved
lib/plugins/print/print.js Outdated Show resolved Hide resolved
expect(print.serverless.cli.consoleLog.called).to.be.equal(true);
expect(YAML.load(message)).to.eql(expected);
});
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Good call. I would add testing some of ${self: ..} to `variables fixture as proposed in other comment

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.

@mnapoli that looks great! I've proposed just one optimization and style improvement to prepared tests.

lib/classes/Variables.test.js Outdated Show resolved Hide resolved
lib/classes/Variables.test.js Outdated Show resolved Hide resolved
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.

That's excellent! Thank you @mnapoli

@medikoo medikoo merged commit 2fdeb51 into serverless:master Sep 24, 2020
@mnapoli mnapoli deleted the fix-3184 branch September 24, 2020 13:47
@walery
Copy link
Contributor

walery commented Sep 30, 2020

Probably the ${self:} was tested because there is example in doc that uses this. I'm not sure how to use it. I've tired to resolve ${self:} with sls print and sls package (in older version where this PR wasn't merged) but both wasn't able to handle this. I think reason for failure is because there is self reference in some objects, but I didn't took deeper look on it. However, I believe ${self:} should be removed from the example in the doc.

There is another case that is uncovered. This is the ${env:} case documented here.

@walery
Copy link
Contributor

walery commented Sep 30, 2020

Btw. in our serverless.yml templates we're using custom variableSyntax and we have our own test cases. I just want to share it as I've already collected the cases some time ago.

From sls doc

https://www.serverless.com/framework/docs/providers/aws/guide/variables/

${self:}
${self:custom.newService.service}-export
${opt:stage, 'dev'}
${sls:instanceId}
${env:SOME_VAR}
${env:}
${opt:stage}
${cf:another-service-dev.functionPrefix}
${cf:another-stack.functionPrefix}
${cf.us-west-2:another-service-dev.functionPrefix}
${s3:myBucket/myKey}
${ssm:/path/to/service/id}
${ssm:/path/to/secureparam~true}
${ssm:/path/to/stringlistparam~split}
${ssm:/aws/reference/secretsmanager/secret_ID_in_Secrets_Manager~true}
${file(../myCustomFile.yml)}
${file(../myCustomFile.yml):globalSchedule}
${file(../myCustomFile.js):schedule.ten}
${env:${opt:stage}_arn}
${opt:stage, self:provider.stage}
${opt:region, 'us-west-1'}

From aws Fn::Sub syntax

https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/pseudo-parameter-reference.html
and
https://docs.aws.amazon.com/AWSCloudFormation/latest/UserGuide/intrinsic-function-reference-sub.html

${MyResource}
${MyResource.Arn}
${MyResource.Arn.Port}
${AWS::AccountId}
${AWS::NotificationARNs}
${AWS::NoValue}
${AWS::Partition}
${AWS::Region}
${AWS::StackId}
${AWS::StackName}
${AWS::URLSuffix}

@medikoo
Copy link
Contributor

medikoo commented Sep 30, 2020

@walery great thanks for that info. I believe it's some unfinished examples, at least it's hard for me to understand what's the use in ${self:}

I've opened a dedicated issue to handle that: #8306

tusbar referenced this pull request in svdgraaf/serverless-pseudo-parameters Oct 1, 2020
Added Deprecation notice
@damien-monni
Copy link

damien-monni commented Nov 6, 2020

Hi, noob question here: I tried to use ${AWS::AccountId} in my template file to inject it in an environment variable but it did not work:

environment:
  ACCOUNT_ID: ${AWS::AccountId}

process.env.ACCOUNT_ID is equal to ${AWS::AccountId} string, it is not replaced by my AWS Account ID.

Am I doing something wrong? I am using serverless 2.4.0.

@mnapoli
Copy link
Contributor Author

mnapoli commented Nov 6, 2020

I think it should be ACCOUNT_ID: !Sub '${AWS::AccountId}' because it's not a serverless variable, and CloudFormation needs !Sub to process it.

walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021
walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021
walery added a commit to walery/serverless that referenced this pull request Feb 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug/design Functionality design flaw cat/variable
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Variables syntax collides with AWS params syntax
5 participants