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

Enable merge invoke. #270

Merged
merged 1 commit into from
Feb 4, 2025
Merged

Enable merge invoke. #270

merged 1 commit into from
Feb 4, 2025

Conversation

brandonpollack23
Copy link
Contributor

@brandonpollack23 brandonpollack23 commented Jan 31, 2025 •
edited
Loading

Fixes pulumi/pulumi#18356

Related #65

@brandonpollack23 brandonpollack23 force-pushed the bpollack/implement_merge branch from 0cca2c0 to ec33a85 Compare January 31, 2025 16:55
@brandonpollack23 brandonpollack23 changed the title [Draft] enable merge invoke. Enable merge invoke. Jan 31, 2025
@brandonpollack23 brandonpollack23 force-pushed the bpollack/implement_merge branch from ec33a85 to 3f19c29 Compare January 31, 2025 16:57
@brandonpollack23 brandonpollack23 marked this pull request as ready for review January 31, 2025 16:57
@brandonpollack23 brandonpollack23 requested a review from a team as a code owner January 31, 2025 16:57
@brandonpollack23 brandonpollack23 force-pushed the bpollack/implement_merge branch from 3f19c29 to 4dc0a22 Compare January 31, 2025 16:58
Frassle
Frassle requested changes Jan 31, 2025
View reviewed changes
Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Again, merge is generic it should not even be in pulumi-std it throws away all typing information. This should be a pcl intrinsic.

@brandonpollack23
Copy link
Contributor Author

After looking closer at this I think it isn't generic it's dynamic.

https://developer.hashicorp.com/terraform/language/functions/merge

These objects in the example are dynamic, they can be any kind of map or object, so in that case there is no generic that will necessarily satisfy any requirment unless you manually define a type that happens to be the appropriate merge of two other types

Frassle
Frassle approved these changes Feb 3, 2025
View reviewed changes
Copy link
Member

@Frassle Frassle left a comment

Choose a reason for hiding this comment

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

Talking to Will we'll try going with these std "any" variants for now, I suspect we're going to have to be smarter about types to get good conversions though, but we can test that with seeing how some more top modules convert

@brandonpollack23 brandonpollack23 mentioned this pull request Feb 3, 2025
Merged
@brandonpollack23 brandonpollack23 force-pushed the bpollack/implement_merge branch 4 times, most recently from 87347f9 to b8bd1ad Compare February 4, 2025 15:53
@brandonpollack23 brandonpollack23 force-pushed the bpollack/implement_merge branch from b8bd1ad to 1c2e9f5 Compare February 4, 2025 16:17
@brandonpollack23 brandonpollack23 merged commit ae96a69 into main Feb 4, 2025
5 checks passed
@brandonpollack23 brandonpollack23 deleted the bpollack/implement_merge branch February 4, 2025 16:28
@lunaris lunaris mentioned this pull request Feb 18, 2025
Closed
@lunaris lunaris mentioned this pull request Feb 27, 2025
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Frassle Frassle Frassle approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Implement the merge TF/PCL function
2 participants
@brandonpollack23 @Frassle