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

Invoking synth multiple times with adapter throws errors #674

Open
1 task
sapslaj opened this issue Feb 21, 2024 · 1 comment
Open
1 task

Invoking synth multiple times with adapter throws errors #674

sapslaj opened this issue Feb 21, 2024 · 1 comment
Labels
bug Something isn't working new An issue that has not yet been triaged by maintainers stale An issue or pull request that has not been updated in a very long time

Comments

@sapslaj
Copy link

sapslaj commented Feb 21, 2024

Description

Attempting to invoke synth multiple times causes the adapter to attempt to re-convert existing resources. For most practical cases there's no reason to run synth multiple times, with the major exception being tests. It's not too uncommon to see Testing.synth(stack) multiple times in a test in my experience, so having synth be essentially non-idempotent was a nasty surprise for me.

CDKTF AWS Adapter Version

v0.13.5

AWS CDK Version

v2.128.0

Gist

https://gist.github.com/sapslaj/017ed9ec3265d062e8207df408a0564c

Possible Solutions

The most brute-force approach would be to simply mark any resources that have already been converted during convert() and just ignore them, but that's not super great if you synth, mutate a construct, then run synth again.

Perhaps a better approach would be to remove any existing resource that matches the logical ID and then define a new one. Somewhere right here:

const res = m.resource(scope, logicalId, props);

// pseudo-code, untested

// remove any child that matches the logical ID so we can overwrite it
scope.tryRemoveChild(logicalId);

// create new resource
const res = m.resource(scope, logicalId, props);

Workarounds

I messed around with trying to unregister the Aspect that the adapter uses after it finishes, but that's egregiously fragile and has other downsides.

The best workaround is... to just not synth twice!

// ...

const synthed = Testing.synth(stack);
expect(synthed).toBeValidTerraform();
expect(synthed).toHaveResourceWithProperties(...);

Anything Else?

Yes, this is very much an edge case, but it was quite surprising and took a good hour of debugging before I realized what was happening 😅.

References

No response

Help Wanted

  • I'm interested in contributing a fix myself

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
@sapslaj sapslaj added bug Something isn't working new An issue that has not yet been triaged by maintainers labels Feb 21, 2024
Copy link

Hi there! 👋 We haven't heard from you in 30 days and would like to know if the problem has been resolved or if you still need help. If we don't hear from you before then, I'll auto-close this issue in 30 days.

@github-actions github-actions bot added the stale An issue or pull request that has not been updated in a very long time label Mar 23, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working new An issue that has not yet been triaged by maintainers stale An issue or pull request that has not been updated in a very long time
Projects
None yet
Development

No branches or pull requests

1 participant