Image component in .NET SDK #115
Conversation
|
I added a couple of examples in pulumi/examples#482 to test this change |
|
I'm still not a collaborator on this repo. Admins - please set up the NuGet publishing keys. @CyrusNajmabadi I would appreciate your review, especially in parts of using inputs/outputs - maybe we can simplify them or add helpers to the base library. |
|
@pgavlin can you setup collaborator access for @mikhailshilkov ? |
sdk/dotnet/Docker.cs
Outdated
| // | ||
| // Pick a reasonably distributed number between 0 and 2^30. This will fit as an int32 | ||
| // which the grpc layer needs. | ||
| var streamID = (int)Math.Floor(new Random().NextDouble() * (1 << 30)); |
There was a problem hiding this comment.
put new Random() in a static, and lock around it. i don't believe Random is threadsafe, and it shouldn't be recreated each time.
There was a problem hiding this comment.
Actually, streamID is not needed in current .NET implementation
There was a problem hiding this comment.
Re-introduced streamId and a static random generator for async stream of outputs
sdk/dotnet/Docker.cs
Outdated
| process.StartInfo.RedirectStandardInput = stdin != null; | ||
| process.StartInfo.RedirectStandardOutput = true; | ||
| process.StartInfo.RedirectStandardError = true; | ||
| process.Start(); |
There was a problem hiding this comment.
we might want to consider a try/catch around this code. .net process stuff can be finicky in my experience
There was a problem hiding this comment.
Added a try/catch around the process call, returning (1, ex.ToString()) in case an exception is thrown. Is that what you intended?
| if (stdin != null) | ||
| { | ||
| process.StandardInput.Write(stdin); | ||
| process.StandardInput.Close(); |
There was a problem hiding this comment.
interesting. we need to close this? why?
There was a problem hiding this comment.
otherwise the process might keep waiting for more input. I think there was a similar close in the node sdk
sdk/dotnet/Docker.cs
Outdated
| // this point because Docker uses stderr for both errors and warnings. So, instead, we | ||
| // just collect the messages, and wait for the process to end to decide how to report | ||
| // them. | ||
| var stderr = await process.StandardError.ReadToEndAsync().ConfigureAwait(false); |
There was a problem hiding this comment.
i'm always terrified of stdout/err in .net. is there any chance the async calls could hang because the first one is awaiting completion, but the process isn't completing because it's trying to write to the other, and nothing is reading from it?
In other words, should we kick off two Task.Runs to ensure that the readings will happen concurrently so we don't get any deadlocks?
There was a problem hiding this comment.
that's a valid point... I guess we should do whatever we can to prevent deadlocks
There was a problem hiding this comment.
I realized that the node SDK was broadcasting output lines as they come, while my .NET implementation was waiting for the complete output. Now, I changed to async output consumption based on OutputDataReceived event. This should also address the potential deadlock.
There was a problem hiding this comment.
should we not do this for streams? i.e. why do you still ReadToEndAsync with .StandardError?
There was a problem hiding this comment.
Changed. Are we good to go?
sdk/dotnet/Extensions.cs
Outdated
| public static Input<T?> Unwrap<T>(this Input<T>? input) where T : class | ||
| { | ||
| return input == null ? Output.Create((T?)null) : input.Apply(v => (T?)v); | ||
| } |
There was a problem hiding this comment.
these seems like useful enough to move to the Pulumi layer. can you file issue/PR about that?
There was a problem hiding this comment.
I split the class to Extensions (generic) and DockerExtension (specific). There are a log of potential generic API based on combinations of inputs, lists, maps, unions, nullable/non-nullable, class/struct, plain/output/input, etc. So, I'm torn on the value of my extensions: are they useful enough to warrant the complexity? What are good names for them?
Looking forward to your opinion.
sdk/dotnet/Extensions.cs
Outdated
| .Apply(v => v != null ? new CacheFromUnwrap { Stages = v } : null); | ||
| } | ||
|
|
||
| public static Input<Union<string, DockerBuildUnwrap>> Unwrap(this InputUnion<string, DockerBuild> build) |
There was a problem hiding this comment.
these could be represented with a lower layer extension that operates on an InputUnion<X, Y> and which takes some helper functions to map from an X to an UnwrapX.
sdk/dotnet/Extensions.cs
Outdated
| }); | ||
| } | ||
|
|
||
| public static Input<ImageRegistryUnwrap?> Unwrap(this Input<ImageRegistry>? registry) |
There was a problem hiding this comment.
would love to move this down as per the above discussion.
sdk/dotnet/Utils.cs
Outdated
| arguments.Append(EscapeArguments((string)argEnumerator.Current!)); | ||
| } | ||
|
|
||
| return arguments.ToString(); |
There was a problem hiding this comment.
this entire method looks like it's simply: string.Join(" ", args.Select(EscapeArguments))
sdk/dotnet/Utils.cs
Outdated
| /// </summary> | ||
| private static string EscapeArguments(string argument) | ||
| { | ||
| using var characterEnumerator = argument.GetEnumerator(); |
There was a problem hiding this comment.
i'm not getting the need for manual enumeratoin. You only seem to process a single char at a time. so just do a foreach (var c in argument) ...
| } | ||
|
|
||
| // Append the final " | ||
| escapedArgument.Append('\"'); |
There was a problem hiding this comment.
nit: this duplicates your switch case above. consider a helper.
There was a problem hiding this comment.
What do you mean here?
There was a problem hiding this comment.
basically, ths looks duplicated with the code here: https://github.com/pulumi/pulumi-docker/pull/115/files/45dae59a6d7429ae40a35a0cff89ca290d5b5fc1#diff-6a4e8545028a22013e41ffad57a63628R76
There was a problem hiding this comment.
These lines are a bit different though. I refactored the loops to one-liners.
sdk/dotnet/Extensions.cs
Outdated
| internal static class UnwrapExtensions | ||
| { | ||
| public static Input<ImmutableArray<T>?> Unwrap<T>(this InputList<T>? items) | ||
| => items == null ? Output.Create((ImmutableArray<T>?)null) : items.Apply(v => (ImmutableArray<T>?)v); |
There was a problem hiding this comment.
feels like this should be returning an Output<ImmutableArray<T>?> instead of an Input<ImmutableArray<T>?>.
(Same comment for most of these).
There was a problem hiding this comment.
I restructured all extensions one more time. The common ones are all expressed in terms of ToOutput, Apply, and MapT0/1. The specific ones are Unwrap that rely on common ones.
The only problem is that I need to preserve null-iness: if an input is nullable, I want the output to contain a nullable, and vice versa. It seems that it's not possible to make nullable and non-nullable overloads of otherwise equal methods, so I had to name them ToOutputNullable etc. Not a fan, but I don't see other options yet.
| => union.Apply(v => v.IsT0 ? func(v.AsT0).Apply(v => Union<U, T1>.FromT0(v)) : Output.Create(Union<U, T1>.FromT1(v.AsT1))); | ||
|
|
||
| public static Output<Union<T0, U>> ApplyT1<T0, T1, U>(this Output<Union<T0, T1>> union, Func<T1, Output<U>> func) | ||
| => union.Apply(v => v.IsT1 ? func(v.AsT1).Apply(v => Union<T0, U>.FromT1(v)) : Output.Create(Union<T0, U>.FromT0(v.AsT0))); |
There was a problem hiding this comment.
will you move tehse down later?
There was a problem hiding this comment.
I can move them to Pulumi if we agree on the API.
|
Code LGTM. Can we have a single test just to smoke test this? |
|
Closing in favor of #119 which can run the tests |
Initial support for .NET SDK, mainly focused on the
Imagecomponent