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

TagsAddingAspect example produces Self-referential block #1892

Closed
danielBreitlauch opened this issue Jun 23, 2022 · 10 comments · Fixed by #1986
Closed

TagsAddingAspect example produces Self-referential block #1892

danielBreitlauch opened this issue Jun 23, 2022 · 10 comments · Fixed by #1986
Labels
bug Something isn't working confirmed independently reproduced by an engineer on the team documentation Improvements or additions to documentation size/small estimated < 1 day

Comments

@danielBreitlauch
Copy link
Contributor

danielBreitlauch commented Jun 23, 2022

Community Note

  • Please vote on this issue by adding a 👍 reaction to the original issue to help the community and maintainers prioritize this request
  • Please do not leave "+1" or other comments that do not add relevant new information or questions, they generate extra noise for issue followers and do not help prioritize the request
  • If you are interested in working on this issue or have submitted a pull request, please leave a comment

cdktf & Language Versions

cdktf: 0.11.2
terraform: 1.2.3
typescript

Affected Resource(s)

aws_ssm_parameter but any resource with non empty tags.

Debug Output

The Gist contains the generated terraform json and the error.
Gist

Expected Behavior

Tags should have been read from the resource, merged and readied to the resource.

Actual Behavior

Terraform considers this a self-referential block and aborts.

Steps to Reproduce

  1. Take the example from https://www.terraform.io/cdktf/concepts/aspects
  2. add a ssm parameter
@danielBreitlauch danielBreitlauch added bug Something isn't working new Un-triaged issue labels Jun 23, 2022
@DanielMSchmidt
Copy link
Contributor

const currentTags = node.tags || {}; should be const currentTags = node.tagsInput || {};, we need to update the docs

@DanielMSchmidt DanielMSchmidt added the documentation Improvements or additions to documentation label Jun 23, 2022
@danielBreitlauch
Copy link
Contributor Author

Makes sense. Thx.
And keep up the good work. Cdktf is awesome.

@DanielMSchmidt DanielMSchmidt added confirmed independently reproduced by an engineer on the team size/small estimated < 1 day and removed new Un-triaged issue labels Jun 24, 2022
@danielBreitlauch
Copy link
Contributor Author

I tested it and see another problem.

new SecurityGroup(this, "autoscaling-sg", { [...], tags: { test: "ing"} })

In this case node.tagsInput is empty but node.tags contains just the dict.
Any ideas?

@DanielMSchmidt
Copy link
Contributor

I think since we are not dealing with tokens anymore you should be able to do node.tags = { ...this.tagsToAdd, ...node.tagsInput,}

@danielBreitlauch
Copy link
Contributor Author

This overwrites the existing tags added elsewhere. Like { test: "ing"} above.

@galaabattur
Copy link

I have tried with both approaches but still getting error.
for the
const currentTags = node.tags || {}; node.tags = { ...this.tagsToAdd, ...currentTags };
error says
Error: Cannot add elements to map token, got: {"test1":"test-adding-tags","&{TfToken[TOKEN.3]}":"String Map Token Value"}. You tried to add a value to a referenced map, instead use Fn.merge([yourReferencedMap, { your: 'value' }]). which makes us to use Fn.merge() and Fn.tomap()
However using Fn.merge() and Fn.tomap() leads us to Self-referential block error, which is circular dependency.
Do we have other approaches to handle this?

@ansgarm
Copy link
Member

ansgarm commented Jul 13, 2022

Hi @galaabattur, have you seen the commit from @DanielMSchmidt (back-linked above)?
It updates the docs to solve that issue by using the tagsInput value for the current tags:

// We need to take the input value to not create a circular reference
const currentTags = node.tagsInput || {};
node.tags = { ...this.tagsToAdd, ...currentTags };

Does that solve it for you as well?

@galaabattur
Copy link

galaabattur commented Jul 15, 2022

Hi @ansgarm thank you for the response. I have tried it and have following question. In this case, where would tagsInput be defined, is it in TaggableConstruct type? otherwise typescript will complain that cannot find the property in the TaggableConstruct type, or is it in the cdktf types?
I think the idea is just to have the current tags and merge with new tags and override with current ones. Thank you

@ansgarm
Copy link
Member

ansgarm commented Jul 19, 2022

Hi @galaabattur,
you're right, we might need to add it to the TaggableConstruct type definition in our Aspects example. It is originally defined in the generated bindings (e.g. here for an S3 Bucket).
The input* attributes can be used to reference whatever was passed to the construct (e.g. here the internal _tags attribute is assigned when assigning something to node.tags = .... So this will take the current tags and merge them with the next ones – if you would use node.tags instead of node.tagsInput it would return a reference instead that will work when Terraform is invoked for example (but you can't change that value in your CDKTF program, that's why that caused an error).

Going to re-open this to fix that the Aspects example does not fail with a type error due to tagsInput now being used.

@github-actions
Copy link
Contributor

I'm going to lock this issue 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 similar to this, 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 Nov 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working confirmed independently reproduced by an engineer on the team documentation Improvements or additions to documentation size/small estimated < 1 day
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants