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.

Sign up for GitHub

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

Jump to bottom

feat(lib): Make default property in Fn.lookup optional and introduce Fn.lookupNested #2672

Merged
merged 5 commits into from
Aug 7, 2023
Merged

feat(lib): Make default property in Fn.lookup optional and introduce Fn.lookupNested #2672

merged 5 commits into from
Aug 7, 2023

Conversation

ansgarm
Copy link
Member

@ansgarm ansgarm commented Feb 23, 2023 •
edited
Loading

This PR implements the idea I had and posted in #2670 (comment)

Todos

  • Add docs

@ansgarm ansgarm requested a review from a team as a code owner February 23, 2023 11:11
@ansgarm ansgarm requested review from mutahhir and DanielMSchmidt and removed request for a team February 23, 2023 11:11
@ansgarm ansgarm changed the title feat(lib): Make default property in Fn.lookup optional and introduce Fn.lookupNested feat(lib): Make default property in Fn.lookup optional and introduce Fn.lookupNested Feb 23, 2023
@ansgarm ansgarm force-pushed the move-tf-functions-to-package branch from 9bbcae4 to 4ce5e7f Compare March 2, 2023 09:59
@mutahhir
Copy link
Member

mutahhir commented Mar 6, 2023

Github's acting weird on this PR (showing commits that are from the parent), so I'll wait to review once the parent PR is merged (or this PR is fixed).

@ansgarm ansgarm force-pushed the move-tf-functions-to-package branch from 5723a2c to 332b96a Compare March 8, 2023 08:09
Base automatically changed from move-tf-functions-to-package to main March 8, 2023 08:52
@ansgarm
Copy link
Member Author

ansgarm commented Mar 8, 2023

@mutahhir just rebased this PR – has a much cleaner diff now 👍

mutahhir
mutahhir approved these changes Mar 13, 2023
View reviewed changes
Copy link
Member

@mutahhir mutahhir left a comment

Choose a reason for hiding this comment

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

Approving, nice change!

One thing I'd like to see would be an update to the docs to introduce this, as I'm sure a lot of people would want to use it. Secondly, can I do array accesses for a specific property in here?

@ansgarm ansgarm requested a review from a team as a code owner July 28, 2023 09:41
@ansgarm ansgarm added this pull request to the merge queue Aug 7, 2023
Merged via the queue into main with commit 31953af Aug 7, 2023
60 of 61 checks passed
@ansgarm ansgarm deleted the simpler-property-access branch August 7, 2023 15:40
@github-actions
Copy link
Contributor

github-actions bot commented Sep 7, 2023

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 Sep 7, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@DanielMSchmidt DanielMSchmidt DanielMSchmidt approved these changes

@mutahhir mutahhir mutahhir approved these changes

Assignees
No one assigned
Labels
ci/run-examples ci/run-unit/cdktf size/s
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@ansgarm @mutahhir @DanielMSchmidt @team-tf-cdk