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(lambda): function.addAlias() simplifies Alias creation #20034

Merged
merged 4 commits into from
Apr 28, 2022

Conversation

rix0rrr
Copy link
Contributor

@rix0rrr rix0rrr commented Apr 22, 2022

Deprecate version.addAlias() in favor of a new method called
function.addAlias().

  • When Aliases get created underneath Versions, there's the risk
    that it's currentVersion whose logical ID changes on every
    deployment, causing a (probably failing) replacement of the Alias
    on every function update.
  • The most common use for function.currentVersion is to create an
    Alias for a Function. Might as well take out the middle man.

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

Deprecate `version.addAlias()` in favor of a new method called
`function.addAlias()`.

- When Aliases get created underneath Versions, there's the risk
  that it's `currentVersion` whose logical ID changes on every
  deployment, causing a (probably failing) replacement of the `Alias`
  on every function update.
- The most common use for `function.currentVersion` is to create an
  `Alias` for a Function. Might as well take out the middle man.
@rix0rrr rix0rrr requested a review from a team April 22, 2022 12:54
@rix0rrr rix0rrr self-assigned this Apr 22, 2022
@gitpod-io
Copy link

gitpod-io bot commented Apr 22, 2022

@aws-cdk-automation aws-cdk-automation requested a review from a team April 22, 2022 12:54
@github-actions github-actions bot added the p2 label Apr 22, 2022
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Apr 22, 2022
Copy link
Contributor

@kaizencc kaizencc left a comment

Choose a reason for hiding this comment

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

When Aliases get created underneath Versions, there's the risk
that it's currentVersion whose logical ID changes on every
deployment, causing a (probably failing) replacement of the Alias
on every function update.
The most common use for function.currentVersion is to create an
Alias for a Function. Might as well take out the middle man.

I'm a bit confused here -- see my inline comments below. The above reads
like the common use case for fn.currentVersion.addAlias() will cause
potentially failing replacement of the Alias on every function update. If so,
I'm unclear how the code changes solves this -- I trust you, I just need it
to be pointed out :).

packages/@aws-cdk/aws-lambda/lib/function.ts Outdated Show resolved Hide resolved
@@ -547,6 +547,18 @@ describe('function', () => {
Annotations.fromStack(stack).hasNoWarning('/Default/MyLambda/$LATEST', Match.stringLikeRegexp(warningMessage));
});

test('function.addAlias', () => {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I'm understanding correctly, the issue with fn.currentVersion.addAlias() is that the Alias will be replaced on every function update. I think we should test that fn.addAlias() solves the issue. This test doesn't really tell me anything.

To be honest, I'm not clear how function.addAlias() changes this behavior. To me, it seems like both fn.addAlias() and fn.currentVersion.addAlias() results in the same thing: return addAlias(this, <current-version>, aliasName, options).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, but the this is different! In one case it's the Function and the other it's the Version.

But I think I was mistaken anyway: I thought the Alias would be replaced if the Version object changed... but I'm now pretty convinced it doesn't.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So this is just convenience.

@rix0rrr rix0rrr added the pr-linter/exempt-integ-test The PR linter will not require integ test changes label Apr 25, 2022
Co-authored-by: Kaizen Conroy <36202692+kaizen3031593@users.noreply.github.com>
@rix0rrr rix0rrr requested review from kaizencc and a team April 27, 2022 08:10
@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject89A8053A-LhjRyN9kxr8o
  • Commit ID: 29ec50f
  • Result: SUCCEEDED
  • Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

@mergify
Copy link
Contributor

mergify bot commented Apr 28, 2022

Thank you for contributing! Your pull request will be updated from master and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

@mergify mergify bot merged commit a79bc47 into master Apr 28, 2022
@mergify mergify bot deleted the huijbers/function-addalias branch April 28, 2022 18:30
wphilipw pushed a commit to wphilipw/aws-cdk that referenced this pull request May 23, 2022
)

Deprecate `version.addAlias()` in favor of a new method called
`function.addAlias()`.

- When Aliases get created underneath Versions, there's the risk
  that it's `currentVersion` whose logical ID changes on every
  deployment, causing a (probably failing) replacement of the `Alias`
  on every function update.
- The most common use for `function.currentVersion` is to create an
  `Alias` for a Function. Might as well take out the middle man.


----

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
contribution/core This is a PR that came from AWS. p2 pr-linter/exempt-integ-test The PR linter will not require integ test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants