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: intro "Names.uniqueId()" instead of the deprecated "node.uniqueId" #11166

Merged
merged 23 commits into from
Nov 3, 2020

Conversation

eladb
Copy link
Contributor

@eladb eladb commented Oct 28, 2020

The property node.uniqueId is used throughout our codebase to generate unique names. We are planning to deprecate this API in constructs 10.x (and CDK 2.0) in favor of a node.addr (see aws/constructs#314).

In most cases, these generated names cannot be changed because they will incur breaking changes to deployments. So we have to continue to use the "legacy" unique ID in those cases (new cases should use addr).

To that end, we introduce a utility Legacy.uniqueId() which uses the legacy algorithm and the code base was migrated to use it instead of node.uniqueId which will go away soon.


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

Elad Ben-Israel added 5 commits October 27, 2020 15:26
This new version includes the `node.addr` property which deprecates `uniqueId`.
The property `node.uniqueId` is used throughout our codebase to generate unique names. We are planning to deprecate this API in constructs 10.x (and CDK 2.0) in favor of a `node.addr` (see aws/constructs#314).

In most cases, these generated names cannot be changed because they will incur breaking changes to deployments. So we have to continue to use the "legacy" unique ID in those cases (new cases should use `addr`).

To that end, we introduce a utility `Legacy.uniqueId()` which uses the legacy algorithm and the code base was migrated to use it instead of `node.uniqueId` which will go away soon.
@gitpod-io
Copy link

gitpod-io bot commented Oct 28, 2020

@eladb eladb requested a review from rix0rrr October 28, 2020 08:29
@mergify mergify bot added the contribution/core This is a PR that came from AWS. label Oct 28, 2020
@eladb eladb force-pushed the benisrae/upgrade-constructs branch from afc1780 to e915f8a Compare October 29, 2020 07:17
Base automatically changed from benisrae/upgrade-constructs to master October 29, 2020 08:31
@eladb eladb marked this pull request as ready for review October 29, 2020 08:40
@eladb eladb changed the title chore: intro "Legacy.uniqueId" instead of the deprecated "node.uniqueId" chore: use "Legacy.uniqueId" instead of "node.uniqueId" Oct 29, 2020
@rix0rrr
Copy link
Contributor

rix0rrr commented Oct 29, 2020

I don't really like the name Legacy, because I don't agree new code should exclusively use addr. addr has no semblance of human readability anymore.

Let's categorize where we use uniqueId:

  • As a path component -- fine, I can live with a hash here.
  • As a generated physical ID in case CFN doesn't generate one for us, or we need a stable one -- losing human readability would be less than ideal.

@eladb
Copy link
Contributor Author

eladb commented Oct 29, 2020

As a generated physical ID in case CFN doesn't generate one for us, or we need a stable one -- losing human readability would be less than ideal.

Ideally I'd want those generated physical IDs to use addr under the hood and not the old unique ID algorithm, but I can see the use case for an official way to produce IDs with a human readable portion based on the construct path. Ideally, this should be done by creating a domain-compatible human readable portion and postfixing addr.

I am also okay with being pragmatic here and keeping the current uniqueId algorithm in AWS CDK.

I'll revise to:

import { Names } from 'core';

Names.uniqueId(c);
Names.nodeUniqueId(c.node);

Sounds okay?

@eladb eladb changed the title chore: use "Legacy.uniqueId" instead of "node.uniqueId" fewat: use "Legacy.uniqueId" instead of "node.uniqueId" Oct 29, 2020
@eladb eladb changed the title fewat: use "Legacy.uniqueId" instead of "node.uniqueId" feat: intro "Names.uniqueId()" instead of the deprecated "node.uniqueId" Oct 29, 2020
@eladb eladb added pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes labels Oct 29, 2020
@aws-cdk-automation
Copy link
Collaborator

AWS CodeBuild CI Report

  • CodeBuild project: AutoBuildProject6AEA49D1-qxepHUsryhcu
  • Commit ID: 7191193
  • 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 Nov 3, 2020

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 5e433b1 into master Nov 3, 2020
@mergify mergify bot deleted the benisrae/legacy-uniqueid branch November 3, 2020 16:54
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. pr-linter/exempt-readme The PR linter will not require README changes pr-linter/exempt-test The PR linter will not require test changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants