5758 for C#/.NET #7899

t0yv0 · 2021-09-02T20:49:39Z

Description

Fixes #7945
Part of #5758 for .NET/C#

Checklist

I have added tests that prove my fix is effective or that my feature works

I have updated the CHANGELOG-PENDING file with my change

Yes, there are changes in this PR that warrants bumping the Pulumi Service API version

github-actions · 2021-09-02T20:52:41Z

Diff for pulumi-random with merge commit a1378c5

github-actions · 2021-09-02T20:53:42Z

Diff for pulumi-azuread with merge commit a1378c5

github-actions · 2021-09-02T20:54:57Z

Diff for pulumi-kubernetes with merge commit a1378c5

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

t0yv0 · 2021-09-02T20:55:47Z

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

@@ -13,6 +13,18 @@ public static class ArgFunction
    {
        public static Task<ArgFunctionResult> InvokeAsync(ArgFunctionArgs? args = null, InvokeOptions? options = null)
            => Pulumi.Deployment.Instance.InvokeAsync<ArgFunctionResult>("example::argFunction", args ?? new ArgFunctionArgs(), options.WithVersion());
+
+        public static Output<ArgFunctionResult> Invoke(ArgFunctionOutputArgs? args = null, InvokeOptions? options = null)


It's called .Invoke but .InvokeOutput is also a possibility to match what we have in other languages. Votes?

I'm leaning InvokeOutput too since in other langs we have _output etc, which Luke also preferred.

Other languages have a very different invoke syntax so I'm not sure that should be a large factor. I expect we'd advise this new form by default - unless folks need the result for control flow, so having a simple unambiguous name would be my preference.

Resource constructors return outputs and they don't have Output in their name, so I don't see much difference here.

FWIW, I don't feel strongly about this -- I could go either way. Invoke is the nicer name, but I do worry slightly that Invoke will be seen as the blocking variant of InvokeAsync, which isn't the case.

t0yv0 · 2021-09-02T20:56:12Z

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

@@ -26,6 +38,16 @@ public ArgFunctionArgs()
        }
    }

+    public sealed class ArgFunctionOutputArgs


*OutputArgs naming convention is something we chose here, can chose differently.

I must admit that doesn't sound right to me. Maybe *InputArgs since it has all Input<T> types?

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/Utilities.cs

github-actions · 2021-09-02T21:02:56Z

Diff for pulumi-gcp with merge commit a1378c5

github-actions · 2021-09-02T21:03:57Z

Diff for pulumi-aws with merge commit a1378c5

github-actions · 2021-09-02T21:08:52Z

Diff for pulumi-azure with merge commit a1378c5

justinvp

Some comments, but LGTM otherwise.

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

@@ -13,6 +13,18 @@ public static class ArgFunction
    {
        public static Task<ArgFunctionResult> InvokeAsync(ArgFunctionArgs? args = null, InvokeOptions? options = null)
            => Pulumi.Deployment.Instance.InvokeAsync<ArgFunctionResult>("example::argFunction", args ?? new ArgFunctionArgs(), options.WithVersion());
+
+        public static Output<ArgFunctionResult> Invoke(ArgFunctionOutputArgs? args = null, InvokeOptions? options = null)


pkg/codegen/dotnet/gen.go

pkg/codegen/dotnet/templates.go

pkg/codegen/internal/test/testdata/output-funcs/dotnet/funcWithDefaultValue.cs

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

mikhailshilkov

This looks exciting!

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

@@ -13,6 +13,18 @@ public static class ArgFunction
    {
        public static Task<ArgFunctionResult> InvokeAsync(ArgFunctionArgs? args = null, InvokeOptions? options = null)
            => Pulumi.Deployment.Instance.InvokeAsync<ArgFunctionResult>("example::argFunction", args ?? new ArgFunctionArgs(), options.WithVersion());
+
+        public static Output<ArgFunctionResult> Invoke(ArgFunctionOutputArgs? args = null, InvokeOptions? options = null)


pkg/codegen/internal/test/testdata/dash-named-schema/dotnet/Utilities.cs

mikhailshilkov · 2021-09-03T08:54:10Z

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs

@@ -26,6 +38,16 @@ public ArgFunctionArgs()
        }
    }

+    public sealed class ArgFunctionOutputArgs


I must admit that doesn't sound right to me. Maybe *InputArgs since it has all Input<T> types?

pkg/codegen/internal/test/testdata/output-funcs/dotnet/Assert.cs

...en/internal/test/testdata/simple-resource-schema-custom-pypackage-name/dotnet/ArgFunction.cs

pkg/codegen/internal/test/testdata/output-funcs/dotnet/CodegenTests.cs

pkg/codegen/internal/test/testdata/output-funcs/dotnet/funcWithConstInput.cs

pkg/codegen/internal/test/testdata/output-funcs/dotnet/funcWithDictParam.cs

pkg/codegen/internal/test/testdata/output-funcs/dotnet/listStorageAccountKeys.cs

github-actions · 2021-09-03T15:14:45Z

Diff for pulumi-random with merge commit 32f48fe

github-actions · 2021-09-03T15:14:46Z

Diff for pulumi-azuread with merge commit 32f48fe

github-actions · 2021-09-03T15:15:35Z

Diff for pulumi-random with merge commit 00e7b40

github-actions · 2021-09-03T15:15:57Z

Diff for pulumi-azuread with merge commit 00e7b40

github-actions · 2021-09-03T15:17:51Z

Diff for pulumi-kubernetes with merge commit 32f48fe

github-actions · 2021-09-03T15:19:19Z

Diff for pulumi-kubernetes with merge commit 00e7b40

github-actions · 2021-09-03T15:24:48Z

Diff for pulumi-gcp with merge commit 32f48fe

github-actions · 2021-09-03T15:25:09Z

Diff for pulumi-gcp with merge commit 00e7b40

github-actions · 2021-09-03T15:25:51Z

Diff for pulumi-aws with merge commit 32f48fe

github-actions · 2021-09-03T15:26:14Z

Diff for pulumi-aws with merge commit 00e7b40

github-actions · 2021-09-03T15:28:49Z

Diff for pulumi-azure with merge commit 32f48fe

github-actions · 2021-09-03T15:30:29Z

Diff for pulumi-azure with merge commit 00e7b40

github-actions · 2021-09-03T16:23:42Z

Diff for pulumi-random with merge commit 6997d1d

github-actions · 2021-09-29T22:09:07Z

Diff for pulumi-random with merge commit 69e9075

github-actions · 2021-09-29T22:11:03Z

Diff for pulumi-kubernetes with merge commit 69e9075

justinvp · 2021-09-30T20:39:22Z

pkg/codegen/dotnet/gen.go

+	case input && args && mod.details(t).usedInFunctionOutputVersionInputs:
+		return name + "InputArgs"


This approach to resolving the conflicting name LGTM.

justinvp · 2021-09-30T21:46:10Z

sdk/dotnet/Pulumi/Deployment/Deployment_Invoke.cs

+            // - the provider is responsible for deciding whether the
+            //   `Output<T>` is secret and known, and may add
+            //   additional dependencies
+            //
+            // This means that presence of secrets or unknowns in the
+            // `args` does not guarantee the result is secret or
+            // unknown, which differs from Pulumi SDKs that choose to
+            // implement these invokes via `Apply` (currently Go and
+            // Python).


I thought Node.js will use .apply? Or is this comment just because the Node.js functionality hasn't landed yet?

This difference across language runtimes is unfortunate. Remind me again why we can't implement this via Apply in .NET?

Answered in another comment below. LMK if you think you see a way forward with Apply. If we could deeply cross-convert plain and inputty types we still use Apply. It's looking like quite bit of work though. I considered doing it by serializing and deserializing but at first glance even that is non-trivial with current API.

justinvp · 2021-09-30T21:47:21Z

sdk/dotnet/Pulumi/Resources/InvokeArgs.cs

@@ -13,10 +13,6 @@ public abstract class InvokeArgs : InputArgs

        private protected override void ValidateMember(Type memberType, string fullName)


I'd have to double check, but after removing the implementation below, I suspect no subclass actually has an implementation and ValidateMember could be nuked entirely. But that can be cleaned up separately.

I'll try that.

t0yv0 · 2021-09-30T22:09:33Z

pkg/codegen/internal/test/testdata/output-funcs-tfbridge20/dotnet/GetAmiIds.cs

+        /// are several valid keys, for a full reference, check out
+        /// [describe-images in the AWS CLI reference][1].
+        /// </summary>
+        public InputList<Inputs.GetAmiIdsFilterInputArgs> Filters


@justinvp this is the pointer to the counter-example where Apply approach broke down.

Using terminology from the doc by @pgavlin : https://github.com/pulumi/pulumi/blob/master/developer-docs/architecture/type-system.md#inputshapet

Suppose we have ideal type X, and we have

dotnetType(X) = GetAmiIdsArgs

then dotnetType(inputShape(X)) = GetAmiIdsInvokeArgs

But as part of covering the fields of this record, we discover dotnetType(Y) = Inputs.GetAmiIdsFilterArgs

and dotnetType(inputShape(Y)) = Inputs.GetAmiIdsFilterInputArgs

X.Filters is of type type List<Y>

Now with the Apply approach, we need to build a .NET function Inputs.GetAmiIdsInvokeArgs -> Output<GetAmiIdsArgs>. The previous implementation was not cutting it. It stumbled on auto-converting Inputs.GetAmiIdsFilterInputArgs to Inputs.GetAmiIdsFilterArgs.

I'd continue arguing that codegen or else SDK needs to reliably be able to do conversions of the form inputShape(X) -> Input<X> in the runtime of the target language, of which the above is a special case :) However poking at the existing SDK it did not sound trivial. I'd have to be reflection, and I don't think it's built yet. Then I noticed that serde has this capability (sort of) and changed the approach.

github-actions · 2021-10-06T14:32:56Z

Diff for pulumi-random with merge commit bebe78d

github-actions · 2021-10-06T14:34:31Z

Diff for pulumi-kubernetes with merge commit bebe78d

github-actions · 2021-10-18T20:25:35Z

Diff for pulumi-azuread with merge commit 2692c71

github-actions · 2021-10-18T20:25:41Z

Diff for pulumi-random with merge commit 2692c71

github-actions · 2021-10-18T20:26:16Z

Diff for pulumi-random with merge commit d742dc2

github-actions · 2021-10-18T20:27:17Z

Diff for pulumi-azuread with merge commit d742dc2

github-actions · 2021-10-18T20:28:04Z

Diff for pulumi-kubernetes with merge commit d742dc2

github-actions · 2021-10-18T20:28:56Z

Diff for pulumi-gcp with merge commit d742dc2

github-actions · 2021-10-18T20:29:17Z

Diff for pulumi-kubernetes with merge commit 2692c71

github-actions · 2021-10-18T20:29:22Z

Diff for pulumi-gcp with merge commit 2692c71

github-actions · 2021-10-18T20:35:21Z

Diff for pulumi-aws with merge commit 2692c71

github-actions · 2021-10-18T20:37:15Z

Diff for pulumi-azure with merge commit d742dc2

github-actions · 2021-10-18T20:37:46Z

Diff for pulumi-azure with merge commit 2692c71

github-actions · 2021-10-18T20:39:23Z

Diff for pulumi-aws with merge commit d742dc2

github-actions · 2021-10-18T21:14:21Z

Diff for pulumi-azure-native with merge commit 2692c71

github-actions · 2021-10-18T21:20:47Z

Diff for pulumi-azure-native with merge commit d742dc2

t0yv0 mentioned this pull request Sep 2, 2021

5758 fix for .NET #7855

Closed

6 tasks

t0yv0 commented Sep 2, 2021

View reviewed changes

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/ArgFunction.cs Outdated Show resolved Hide resolved

t0yv0 commented Sep 2, 2021

View reviewed changes

pkg/codegen/internal/test/testdata/external-resource-schema/dotnet/Utilities.cs Outdated Show resolved Hide resolved

t0yv0 requested review from mikhailshilkov, pgavlin, lblackstone and justinvp September 2, 2021 20:57

justinvp approved these changes Sep 3, 2021

View reviewed changes

mikhailshilkov reviewed Sep 3, 2021

View reviewed changes

Fixup cyclic-types

78f9ba4

justinvp reviewed Sep 30, 2021

View reviewed changes

t0yv0 commented Sep 30, 2021

View reviewed changes

Merged master

9abd8c0

This was referenced Oct 6, 2021

5758 for Node JS #8047

Merged

Minimally extend .NET SDK to support 5758 #8142

Merged

Update C# examples to use the new F.Invoke feature pulumi/examples#1093

Merged

t0yv0 added 3 commits October 18, 2021 16:02

Merged master

289b68c

Accept codegen

0168cd3

NOTE we now require .NET SDK 3.15 or higher

ae9f769

t0yv0 merged commit e710125 into master Oct 18, 2021

pulumi-bot deleted the t0yv0/5758-dotnet-2 branch October 18, 2021 22:18

		case input && args && mod.details(t).usedInFunctionOutputVersionInputs:
		return name + "InputArgs"

		@@ -13,10 +13,6 @@ public abstract class InvokeArgs : InputArgs

		private protected override void ValidateMember(Type memberType, string fullName)

5758 for C#/.NET #7899

5758 for C#/.NET #7899

Conversation

t0yv0 commented Sep 2, 2021 • edited by emiliza Loading

Description

Checklist

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

Choose a reason for hiding this comment

This comment was marked as resolved.

This comment was marked as resolved.

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

github-actions bot commented Sep 2, 2021

justinvp left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

mikhailshilkov left a comment

Choose a reason for hiding this comment

This comment was marked as resolved.

Choose a reason for hiding this comment

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 3, 2021

github-actions bot commented Sep 29, 2021

github-actions bot commented Sep 29, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Oct 6, 2021

github-actions bot commented Oct 6, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

github-actions bot commented Oct 18, 2021

t0yv0 commented Sep 2, 2021 •

edited by emiliza

Loading