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 Step 2 C# Example #3208

Merged
merged 2 commits into from May 7, 2020
Merged

Update Step 2 C# Example #3208

merged 2 commits into from May 7, 2020

Conversation

efleming18
Copy link
Contributor

I tried following this example and ran in to multiple potential issues.

  • The initial tutorial puts all of the code in to the MyStack.cs class, and the run is only using that file for all of the updates. The description in Step 2, describes putting the code in to MyStack.cs but the code snippet is putting everything in Program.cs.
  • Even if using Program.cs, it would need to be Deployment.RunAsync, there's a compiler error with a straight copy/paste
  • There's a syntax issue when specifying the SecurityGroupIngressArgs, there's an extra set of curly brackets that I've removed
  • The line for specifying the ami.Id will not work unless you get the .Result of the GetAmi.InvokeAsync. Without, ami.Id just returns 1. Is there a better way to fix this? I always try to avoid doing .Result, but not sure how you want to approach that in this case.
  • I added the two [Output] properties for PublicIp and PublicDns
  • I also changed the use of SecurityGroups which is apparently deprecated to use VpcSecurityGroupIds. This appears to have worked like normal, but would definitely like for you to confirm.

Please feel free to take another approach with this, this is just what I had to do in order to get the tutorial working as I believe it was intended to. I'm also happy to make any adjustments to this PR if that would help you out.

If I did something wrong and the tutorial is working as intended the way it is on the site right now, please let me know so I can adjust for future reference.

Thanks!

I tried following this example and ran in to multiple potential issues. 
- The initial tutorial puts all of the code in to the `MyStack.cs` class, and the run is only using that file for all of the updates. The description in Step 2, describes putting the code in to `MyStack.cs` but the code snippet is putting everything in `Program.cs`.
- Even if using `Program.cs`, it would need to be `Deployment.RunAsync`, there's a compiler error with a straight copy/paste
- There's a syntax issue when specifying the `SecurityGroupIngressArgs`, there's an extra set of curly brackets that I've removed
- The line for specifying the `ami.Id` will not work unless you get the `.Result` of the `GetAmi.InvokeAsync`. Without, `ami.Id` just returns `1`. Is there a better way to fix this? I always try to avoid doing `.Result`, but not sure how you want to approach that in this case.
- I added the two `[Output]` properties for `PublicIp` and `PublicDns`

Please feel free to take another approach with this, this is just what I had to do in order to get the tutorial working as I believe it was intended to. I'm also happy to make any adjustments to this PR if that would help you out. 

If I did something wrong and the tutorial _is_ working as intended the way it is on the site right now, please let me know so I can adjust for future reference.

Thanks!
@leezen leezen requested a review from mikhailshilkov May 5, 2020 02:26
@mikhailshilkov
Copy link
Member

@efleming18 Thanks for the PR! These tutorials are unfortunately outdated and didn't get any love after the recent changes. I believe we can copy the code from https://github.com/pulumi/examples/blob/master/aws-cs-webserver/WebServerStack.cs - this would avoid using .Result, for example. Could you please update your PR to match? Thank you!

@efleming18
Copy link
Contributor Author

I added some updates based on your feedback, @mikhailshilkov - if there's anything else I need to do for it, please let me know.

Copy link
Member

@mikhailshilkov mikhailshilkov left a comment

Choose a reason for hiding this comment

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

Thanks!

@mikhailshilkov mikhailshilkov merged commit c0863aa into pulumi:master May 7, 2020
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.

None yet

2 participants