add default output for AWS account Id variable in CF template as discussed in #820

[aletheia]

Pull Request Template ------------------------

• Check out our Pull Request Guidelines here: https://github.com/serverless/serverless/blob/v1.0/CONTRIBUTING.md

Status:

Ready/In Development/Hold

Description:

Todos:

• Write tests

…iable is filled correctly when generating default template. Thanks to @erikerikson for all support.

[flomotlik]

Hi @aletheia , could you walk me through the use case for this? I'm not sure where and how we're using this right now.

[erikerikson]

Hey @flomotlik - the general use case being enabled is that different stages can belong in different accounts and sometimes you need the account ID to properly compose ARNs and the like. A significant subset of the community has this pattern. I'm one of them so I may be biased.

Right now, this is enabled by hacking the iamRoleArnLambda variable output by the CFT (here).

This change grabs the account ID officially and because of how the code is written uses that official source when available. Because the code will still use the iamRoleArnLambda, it maintains support for older project artifacts. It is a smart improvement to my original, linked, PR.

[ac360]

Nice @aletheia. @flomotlik Overall, this is what we need: #1468. What do you think of the template in that issue @erikerikson and @aletheia?

Currently, V.1 returns no info after deployment. It's completely unhelpful. By utilizing CloudFormation Output Variables in the default template we can present users with useful information after deployment.

This is a big priority imo. The basic AWS deployment process feels incomplete without this.

[pmuens]

Let me chime in on this and point you to this piece of code https://github.com/serverless/serverless/blob/v1.0/lib/plugins/aws/deploy/compile/events/apiGateway/lib/methods.js#L37-L44 which might also benefit from this PR / the move into the "more Output variables in CloudFormation" direction.

It might be also useful for future implementations.

[flomotlik]

@pmuens In the specific code you linked couldn't we simply use Ref instead of building our own ARN? This looks like its left over from the old code that went through the API so we needed to get the ARN somehow, but now we should be able to use Cloudformation functions for it shouldn't we?

the general use case being enabled is that different stages can belong in different accounts and sometimes you need the account ID to properly compose ARNs and the like. A significant subset of the community has this pattern. I'm one of them so I may be biased

@erikerikson due to our move to Cloudformation shouldn't you be able to simply use a Ref here instead of having to build your own ARN? I worry that including the Account ID as an Output variable leads to a bad pattern of building your own ARNs instead of Relying on Cloudformation to figure this out automatically?

@erikerikson @aletheia could you describe a specific use case that you have at the moment where you're using this to build your own ARN? Especially in case this isn't covered by anything CloudFormation could do (or already does). Would really help in understanding your needs here in more detail.

[pmuens]

Yep. Good points @flomotlik.

I would also like to keep the Output section as lean as possible and encourage CloudFormation usage whenever possible. I feel bad about the code I linked and I think that this could be resolved elegantly and easily with CloudFormation.

[erikerikson]

@flomotlik you make some great points. Thanks for pointing out my disconnect between the old and new. I was definitely thinking in terms of pre-v1. Sorry for that. The issue came to light because of side effects from my PR that @athletia referenced above. This was definitely to avoid bad behavior internally to SLS and instead promote the good.

That said, I know that there have been cases, particularly in roles, that we have had to use the accountId. That said, I imagine that with the newer style we could use inter-CFT references to pull the necessary info, I just haven't done that yet. That said, it can provide a lot of value (particularly from a least privilege standpoint) to separate role creations (IAM activity) from the rest of the use of SLS and doing that might require ARNs or at least output variables that dependent CFTs could pick up.

@ac360 I missed the actual template in #1468. That said, the shape of the idea seemed very good.

[aletheia]

I am unsure about what you mean with "due to our move to Cloudformation”. Could you share more info about that?
Right now, with release 0.5.x we rely on account id if we wish to direct reference an endpoint in gateway via AWS SDK.
Also SLS actually does this when deploying endpoints and lambda functions.
What am I missing?

On 14 lug 2016, at 12:53, Florian Motlik notifications@github.com wrote:

@pmuens https://github.com/pmuens In the specific code you linked couldn't we simply use Ref instead of building our own ARN? This looks like its left over from the old code that went through the API so we needed to get the ARN somehow, but now we should be able to use Cloudformation functions for it shouldn't we?

the general use case being enabled is that different stages can belong in different accounts and sometimes you need the account ID to properly compose ARNs and the like. A significant subset of the community has this pattern. I'm one of them so I may be biased

@erikerikson https://github.com/erikerikson due to our move to Cloudformation shouldn't you be able to simply use a Ref here instead of having to build your own ARN? I worry that including the Account ID as an Output variable leads to a bad pattern of building your own ARNs instead of Relying on Cloudformation to figure this out automatically?

@erikerikson https://github.com/erikerikson @aletheia https://github.com/aletheia could you describe a specific use case that you have at the moment where you're using this to build your own ARN? Especially in case this isn't covered by anything CloudFormation could do (or already does). Would really help in understanding your needs here in more detail.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #1540 (comment), or mute the thread https://github.com/notifications/unsubscribe/AAn5_7bj8SWZyZKk7adx4uOB4blT4QTdks5qVhVHgaJpZM4JJuBn.

Luca Bianchi PhD
bianchi.luca@gmail.com mailto:luca.bianchi@neosperience.com

[flomotlik]

@aletheia we've been working on v1 of the Framework for a while and started releasing fist alphas. This new version is built completely on Cloudformation: http://blog.serverless.com/serverless-v1-0-alpha1-announcement/

[aletheia]

LoL I’ve read the announcement, but I missed the point on relying completely on CF. Now I am a bit worried about deployment time, but it is better to discuss this on Gitter. Thank you.

On 15 lug 2016, at 10:28, Florian Motlik notifications@github.com wrote:

@aletheia https://github.com/aletheia we've been working on v1 of the Framework for a while and started releasing fist alphas. This new version is built completely on Cloudformation: http://blog.serverless.com/serverless-v1-0-alpha1-announcement/ http://blog.serverless.com/serverless-v1-0-alpha1-announcement/
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #1540 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAn5_-py2duFkH6UrUUEWrbcLJK6gtsaks5qV0SpgaJpZM4JJuBn.

Luca Bianchi PhD
bianchi.luca@gmail.com mailto:luca.bianchi@neosperience.com

[pmuens]

Hey @aletheia
@ac360 just did a quick benchmark regarding deployment time. You can see the results here: #1486 (comment)

Just keep in mind that this is was a quick benchmark.

[flomotlik]

That said, I imagine that with the newer style we could use inter-CFT references to pull the necessary info, I just haven't done that yet

@erikerikson yup let me know if any questions come up there. And as we support Cloudformation natively inside of the serverless.yml adding an output variable like this to it is super simple. I don't think we have to add it by default, as my assumption is it will only be used rarely, but its absolutely possible (and super simple) to get it in there.

@aletheia I'm going to close this PR as it looks like this question is resolved? Please let me know in case I'm wrong here and we can reopen. We should have also opened all other issues that were discussed in here with new options, e.g. #1468

[aletheia]

Yeah, I think it is ok
Thank you for all the clarifications

On 15 lug 2016, at 16:00, Florian Motlik notifications@github.com wrote:

That said, I imagine that with the newer style we could use inter-CFT references to pull the necessary info, I just haven't done that yet

@erikerikson https://github.com/erikerikson yup let me know if any questions come up there. And as we support Cloudformation natively inside of the serverless.yml adding an output variable like this to it is super simple. I don't think we have to add it by default, as my assumption is it will only be used rarely, but its absolutely possible (and super simple) to get it in there.

@aletheia https://github.com/aletheia I'm going to close this PR as it looks like this question is resolved? Please let me know in case I'm wrong here and we can reopen. We should have also opened all other issues that were discussed in here with new options, e.g. #1468 #1468
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub #1540 (comment), or mute the thread https://github.com/notifications/unsubscribe-auth/AAn5_z-Zr-L6FrzyRxQB2U7zwl6iosl7ks5qV5KLgaJpZM4JJuBn.

Luca Bianchi PhD
bianchi.luca@gmail.com mailto:luca.bianchi@neosperience.com

[erikerikson]

So... I think this should be directed back to the master branch as a pre-v1 release fix?

[flomotlik]

@erikerikson we decided that 0.5.6 will be our last release in pre-v1 so we can fully focus on V1 going forward.