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(lib): change the namespace of encoded string, list and number tokens #1148

Merged
merged 5 commits into from
Oct 19, 2021

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Oct 15, 2021

To make sure CDK for Terraform tokens don't conflict with AWS CDK
tokens we change the namespace of string, list and number tokens.
For string tokens we use e.g. ${TFToken[1]} instead of ${Token[1]}.
Likewise for list tokens (they will become #{TFToken[1]}).
For number tokens we change the left part of the bitmask from 0xfbff
to 0xfdff (which results in flipping bit 25 instead of bit 26).

This change makes sure that tokens don't get accidentally resolved
when e.g. a CDKTF token gets passed to an AWS CDK construct. However,
it won't achieve full interoperability between the two token systems.

@skorfmann
Copy link
Contributor

Makes sense.

For number tokens we change the left part of the bitmask from 0xfbff
to 0xfdff (which results in flipping bit 25 instead of bit 26).

Not sure about the implications of this.

This change makes sure that tokens don't get accidentally resolved
when e.g. a CDKTF token gets passed to an AWS CDK construct. However,
it won't achieve full interoperability between the two token systems.

Don't think Token interop should be a goal at the moment.

@ansgarm
Copy link
Member Author

ansgarm commented Oct 15, 2021

Not sure about the implications of this.

It should just be a different bit mask 😁 I only wanted to explain what happens by using d instead of b because it might not look obvious why I didn't choose e.g. c or a instead. In the end it shouldn't matter.

Don't think Token interop should be a goal at the moment.

Agree. I only wanted to be very clear about it, so I included that comment in the commit message.

…kens

To make sure CDK for Terraform tokens don't conflict with AWS CDK
tokens we change the namespace of string, list and number tokens.
For string tokens we use e.g. `${TFToken[1]}` instead of `${Token[1]}`.
Likewise for list tokens (they will become `#{TFToken[1]}`).
For number tokens we change the left part of the bitmask from `0xfbff`
to `0xfdff` (which results in flipping bit 25 instead of bit 26).

This change makes sure that tokens don't get accidentally resolved
when e.g. a CDKTF token gets passed to an AWS CDK construct. However,
it won't achieve full interoperability between the two token systems.
@ansgarm ansgarm marked this pull request as ready for review October 18, 2021 09:02
@skorfmann
Copy link
Contributor

One build was cancelled due to Docker rate limits

  Warning: Docker pull failed with exit code 1, back off 2.751 seconds before retry.
  /usr/bin/docker pull docker.mirror.hashicorp.services/hashicorp/jsii-terraform
  Error: The operation was canceled.

Gonna merge anyway.

@skorfmann skorfmann merged commit 09b75ce into main Oct 19, 2021
@skorfmann skorfmann deleted the change-token-namespace branch October 19, 2021 14:21
@ansgarm
Copy link
Member Author

ansgarm commented Oct 19, 2021

🎉🎉🎉

@github-actions
Copy link
Contributor

github-actions bot commented Dec 6, 2022

I'm going to lock this pull request because it has been closed for 30 days. This helps our maintainers find and focus on the active issues. If you've found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 6, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants