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

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

Merged
merged 18 commits into from
Nov 12, 2021
Merged

Conversation

t0yv0
Copy link
Member

@t0yv0 t0yv0 commented Oct 6, 2021

Context: pulumi/pulumi#7948

Updates C# examples to use F.Invoke feature.

This PR will not compile and be ready to merge until:

  1. Minimally extend .NET SDK to support 5758 pulumi#8142 is released in a new version of Pulumi .NET SDK
  2. 5758 for C#/.NET pulumi#7899 is merged in pulumi/pulumi
  3. providers are updated to take advantage of 7899
  4. Fix 8322 pulumi#8339 fix released

It is ready for review however, only minimal changes to build refs are expected once the prereqs land.

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 6, 2021

Done 28/295
Next aws-django-voting-app/Pulumi.yaml
Done for C# 28/34
Next C# azure-cs-net5-aks-webapp/Pulumi.yaml

To be continued.

@t0yv0 t0yv0 changed the title WIP upgrading examples to use the new fn.Output feature WIP upgrading C# examples to use the new fn.Output feature Oct 7, 2021
@t0yv0 t0yv0 marked this pull request as ready for review October 7, 2021 15:04
@t0yv0 t0yv0 changed the title WIP upgrading C# examples to use the new fn.Output feature Update C# examples to use the new F.Invoke feature Oct 7, 2021
@mikhailshilkov
Copy link
Member

For a second, I thought I missed that we landed .NET invokes already. These changes look great!

@t0yv0
Copy link
Member Author

t0yv0 commented Oct 8, 2021

@mikhailshilkov it's fairly close, I expect next planned release to land SDK changes and the cycle after that land the providers changes.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 3, 2021

CI indicates pulumi-aws has not upgraded yet, but this should be coming shortly.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 4, 2021

I think the remaining CI failure is legitimate, it's indicative of preview failure; waiting on this fix: pulumi/pulumi#8339

@t0yv0 t0yv0 requested a review from a team November 9, 2021 15:21
var vpcId = vpc.Apply(vpc => vpc.Id);
var subnet = vpcId.Apply(id => Ec2.GetSubnetIds.InvokeAsync(new Ec2.GetSubnetIdsArgs {VpcId = id}));
var subnetIds = subnet.Apply(s => s.Ids);
var vpcId = Ec2.GetVpc.Invoke(new Ec2.GetVpcInvokeArgs { Default = true })
Copy link
Member

Choose a reason for hiding this comment

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

Invokes normally run at preview time right? So while this looks neater and smaller it's unnecessarily moved to Output space?

Copy link
Member

Choose a reason for hiding this comment

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

What's bad about that?

Copy link
Member

Choose a reason for hiding this comment

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

Well in general Output is more constrained in what you can do with it than Task. I don't think it really matters here because the inputs are always going to be known and so these invokes will run anyway just wondering out loud if this is going to make things more difficult with preview constraints as even more stuff is in Outputs.

Copy link
Member Author

Choose a reason for hiding this comment

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

We had a similar debate in TypeScript with Pat eventually resolved it to the rule "if the change makes Output appear that was not there before, it's a bad change". I'm happy to roll it back.

Copy link
Member Author

Choose a reason for hiding this comment

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

Invokes should not be running in preview AFAIK.

Copy link
Member

Choose a reason for hiding this comment

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

Invokes should run in preview if the Inputs are known though?

Copy link
Member

Choose a reason for hiding this comment

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

Old-style invokes totally run in preview unless inside an unresolved apply

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 10, 2021

Undoing classic-azure examples as C# feature has not rolled out yet.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 10, 2021

Confirming CI failure fixed on 3.17.1 but something is busted with the release as release automation is picking up 3.17.0.

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 10, 2021

So @Frassle check this out.

using System;
using Pulumi;
using Ec2 = Pulumi.Aws.Ec2;

class EksStack : Stack
{
    public EksStack()
    {
        //NewVersion();
        OldVersion();
    }

    public void NewVersion()
    {
        // Read back the default VPC and public subnets, which we will use.
        var vpc = Ec2.GetVpc.Invoke(new Ec2.GetVpcInvokeArgs { Default = true });

        PrintIsKnown(vpc);

        var vpcId = vpc.Apply(vpc =>
        {
            Console.WriteLine($"vpc.Id={vpc.Id}");
            return vpc.Id;
        });

        PrintIsKnown(vpcId);

        var subnetIds = Ec2.GetSubnetIds.Invoke(new Ec2.GetSubnetIdsInvokeArgs { VpcId = vpcId })
            .Apply(s =>
            {
                Console.WriteLine($"s.Ids={s.Ids}");
                return s.Ids;
            });

        PrintIsKnown(subnetIds);
    }

    public void OldVersion()
    {
        var vpc = Output.Create(Ec2.GetVpc.InvokeAsync(new Ec2.GetVpcArgs
        {
            Default = true
        }));

        PrintIsKnown(vpc);

        var vpcId = vpc.Apply(vpc =>
        {
            Console.WriteLine($"vpc.Id={vpc.Id}");
            return vpc.Id;
        });

        PrintIsKnown(vpcId);

        var subnet = vpcId.Apply(id => Ec2.GetSubnetIds.InvokeAsync(new Ec2.GetSubnetIdsArgs {VpcId = id}));

        var subnetIds = subnet.Apply(s =>
        {
            Console.WriteLine($"s.Ids={s.Ids}");
            return s.Ids;
        });

        PrintIsKnown(subnetIds);
    }


    public void PrintIsKnown<T>(Output<T> output)
    {
        Output.Create(Pulumi.Utilities.OutputUtilities.GetIsKnownAsync(output)).Apply(isKnown =>
        {
            Console.WriteLine($"IsKnown: {isKnown}");
            return 0;
        });
    }

}

Both versions print the same output in pulumi preview:

   vpc.Id=vpc-b6f1edcc
    IsKnown: True
    IsKnown: True
    s.Ids=System.Collections.Immutable.ImmutableArray`1[System.String]
    IsKnown: True

@t0yv0
Copy link
Member Author

t0yv0 commented Nov 10, 2021

So yes it sounds like invokes would run in preview if known. The two forms seem equivalent.

@t0yv0 t0yv0 requested a review from Frassle November 11, 2021 22:31
@t0yv0
Copy link
Member Author

t0yv0 commented Nov 11, 2021

Unbelievable, green checkmark from CI! 🙂

@t0yv0 t0yv0 requested a review from a team November 12, 2021 15:47
@t0yv0 t0yv0 merged commit ce1f4a9 into master Nov 12, 2021
@t0yv0 t0yv0 deleted the t0yv0/fn-outputs branch November 12, 2021 16:02
dixler pushed a commit that referenced this pull request Jan 21, 2022
* Update aws-cs-eks

* Update aws-cs-fargate

* Updated aws-cs-webserver

* Update azure-cs-aks-cosmos-helm

* Update azure-cs-aks-helm

* Update azure-cs-aks

* Updated azure-cs-appservice-docker

* Update azure-cs-appservice

* Update azure-cs-cosmosdb-logicapp

* Update azure-cs-credential-rotation-one-set

* Update azure-cs-functions

* Updated azure-cs-net5-aks-webapp

* Update aws-cs-fargate/Infra

* Update testing-unit-cs

* No-op change to trigger CI

* Force 3.17.1 upgrade

* Trigger CI again

* Fix indent
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants