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): Expand Output value types #1145

Merged
merged 6 commits into from
Nov 12, 2021

Conversation

jsteinich
Copy link
Collaborator

Fixes #1088

Terraform output values can be assigned any expression according to https://www.terraform.io/docs/language/values/outputs.html#declaring-an-output-value.

Since we have started having a type to represent expressions, I thought it would make sense to start using it. I added a few things in so that we wouldn't reduce what could be passed to an output. We don't yet have a complete list; however, we do have any which can help cover missing cases.

I also added in ITerraformDependable to just output values to cover outputting an entire resource or module. I assume data sources can also be used that way, but if not, we can easily narrow it. I'm not sure if these can be used in general expressions, but if so, can move to there.

Draft because no new tests yet.

@ansgarm
Copy link
Member

ansgarm commented Oct 27, 2021

Hi @jsteinich! Should I help write some tests so we can merge this? Or do you already have some work in progress tests lying around?

@jsteinich
Copy link
Collaborator Author

Hi @jsteinich! Should I help write some tests so we can merge this? Or do you already have some work in progress tests lying around?

You can if you want; otherwise, I'll get to them at some point. I've just been prioritizing some other things since I created this.

@ansgarm ansgarm force-pushed the expanded_output_types branch from 59798cf to cb1b28d Compare October 28, 2021 13:08
@ansgarm
Copy link
Member

ansgarm commented Oct 28, 2021

@jsteinich fyi: I rebased your branch onto main and force pushed, hope that is okay with you 😇

@ansgarm ansgarm force-pushed the expanded_output_types branch from 9e1b526 to 122be53 Compare October 28, 2021 14:22
@ansgarm
Copy link
Member

ansgarm commented Oct 28, 2021

@jsteinich I added a testcase for passing a complete resource to an output (which will follow the isITerraformDependable path in the code).
Is there another test case you'd like to see or had in mind?

@ansgarm ansgarm force-pushed the expanded_output_types branch from 122be53 to aedcee7 Compare October 28, 2021 14:34
@jsteinich
Copy link
Collaborator Author

@jsteinich I added a testcase for passing a complete resource to an output (which will follow the isITerraformDependable path in the code). Is there another test case you'd like to see or had in mind?

Thanks. I added a couple more tests to pick up some of the expression expansion as well.

@jsteinich jsteinich marked this pull request as ready for review October 29, 2021 02:48
@skorfmann
Copy link
Contributor

Is this related to to #1279 ? If not, can we piggyback on this or should we create a new PR?

@jsteinich
Copy link
Collaborator Author

Is this related to to #1279 ? If not, can we piggyback on this or should we create a new PR?

Yeah, we should fix that here (it isn't currently fixed).

@jsteinich
Copy link
Collaborator Author

I think we'll need to do something similar to TfExpression.markAsInner to correctly synthesis the value for an output for #1279. I can take care of that in the next couple of days.

@jsteinich jsteinich force-pushed the expanded_output_types branch from 4b7a1a6 to 9b750cd Compare November 12, 2021 02:02
@jsteinich
Copy link
Collaborator Author

Alright I updated the synthesis in a way that should take care of #1279 (working in a very similar test case) as well as a couple other cases that users could run into.

Copy link
Contributor

@skorfmann skorfmann left a comment

Choose a reason for hiding this comment

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

Cool, thank you!

@github-actions
Copy link
Contributor

github-actions bot commented Dec 5, 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 5, 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.

TerraformOutput fails to output resource object
3 participants