Fix up Azure Functions sample for publishing #531
Conversation
|
cloned and tried deploying - worked like a champ. ship it. |
|
Adding back |
Yes, as soon I saw the failing CI I realized I opened that can of worms again 😭. Let me know if you'd like any help or to split up the changes in this PR. |
|
Wow, that worked 😮 |
Should we kick CI a couple of times to sanity check? |
Yeah I don't even think the logic I added is being hit but instead it's working because now |
samples/AspireWithAzureFunctions/ImageGallery.AppHost/Program.cs
Outdated
Show resolved
Hide resolved
| return Results.NotFound(); | ||
| } | ||
|
|
||
| var properties = (await blobClient.GetPropertiesAsync(cancellationToken: cancellationToken)).Value; |
There was a problem hiding this comment.
Yeah not sure what you're referring to here.
There was a problem hiding this comment.
Wrapping an await in parathesis assumes that it returns a non-null value and when you access it with .Value, you're saying that you assume for certain that the call to .GetPropertiesAsync will happily return. It's cleaner/safer to get the response, check for null and then access the .Value.
IMO every time I see anything that does (await SomethingAsync()).SomeProp, I think of that as a code smell.
There was a problem hiding this comment.
That's what we have nullable ref types for though, and Value isn't marked as nullable, so this should be fine. There's literally nothing of value on the object returned from GetPropertiesAsync other than the Value member.
There was a problem hiding this comment.
Is the return value nullable @DamianEdwards ?
There was a problem hiding this comment.
No, and if it were I'd get a compiler message complaining about how I'm using properties afterwards. The doc comment for Value says it will throw upon access only if the entire response is declared as a nullable response, which it isn't here.
samples/AspireWithAzureFunctions/ImageGallery.Functions/ThumbnailGenerator.cs
Outdated
Show resolved
Hide resolved
samples/AspireWithAzureFunctions/ImageGallery.Functions/ThumbnailGenerator.cs
Outdated
Show resolved
Hide resolved
samples/AspireWithAzureFunctions/ImageGallery.Functions/ThumbnailGenerator.cs
Outdated
Show resolved
Hide resolved
samples/AspireWithAzureFunctions/ImageGallery.Functions/ThumbnailGenerator.cs
Outdated
Show resolved
Hide resolved
| construct.Add(storageAccount.CreateRoleAssignment(StorageBuiltInRole.StorageAccountContributor, construct.PrincipalTypeParameter, construct.PrincipalIdParameter)); | ||
| construct.Add(storageAccount.CreateRoleAssignment(StorageBuiltInRole.StorageBlobDataOwner, construct.PrincipalTypeParameter, construct.PrincipalIdParameter)); | ||
| // Ensure that public access to blobs is disabled | ||
| storageAccount.AllowBlobPublicAccess = false; |
There was a problem hiding this comment.
The default value is false so we could remove this but I think there's value in the explicitness for this sample.
There was a problem hiding this comment.
Yeah I wasn't actually sure as the doc comments are kinda confusing. I interpreted this to mean it defaulted to true?
This PR fixes up a couple of issues with the Azure Functions sample that make it unpublishable:
SkiaSharp.NativeAssets.Linux.NoDependenciespackage to support running SkiaSharp in Linux containers on ACA