-
Notifications
You must be signed in to change notification settings - Fork 759
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
Add 'localDeploy' experimental feature, base extension nuget library #14231
Conversation
807b53b
to
c6804d8
Compare
Test this change out locally with the following install scripts (Action run 9369999284) VSCode
Azure CLI
|
d393a9f
to
da3f298
Compare
this.mainLayer = this.Layers.Single(); | ||
var expectedLayerMediaType = BicepMediaTypes.BicepProviderArtifactLayerV1TarGzip; | ||
this.mainLayer = this.Layers.Where(l => l.MediaType.Equals(expectedLayerMediaType, MediaTypeComparison)).Single(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe out of scope for this PR since the replaced line was using .Single()
, but does this result in an understandable error message if a template author refers to an artifact that violates this expectation?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the code so far is assuming packages to be well-formed - this is something I think we'll need to improve.
ConnectCallback = connectionFactory.ConnectAsync | ||
}; | ||
|
||
return GrpcChannel.ForAddress("http://localhost", new GrpcChannelOptions |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the URL actually used since the channel is connecting over a domain socket? If it isn't, I might make it something unresolvable like never://this-should-not-be-used.wtf
so that the whole thing fails fast and the FQDN jumps out in log messages in case this ever does not work as expected.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is taken from the official docs: https://learn.microsoft.com/en-us/aspnet/core/grpc/interprocess-uds?view=aspnetcore-8.0. I'll add a comment here to explain this, as it's not very clear from the code that it's unused. I'd rather stick with localhost than make up a domain which may confuse people further - especially if I don't think we'd hit this in production.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be slightly more concerned about the failure mode -- http://localhost
is just as likely to connect to the wrong thing as it is to fail to connect, which I would think would be more confusing for users (e.g., it'd be reported as something exotic like a GRPC protocol error or validation exception).
This is a pretty far-fetched scenario, though, so feel free to ignore.
da3f298
to
a7998b2
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Had one question about testability but am not sure if it's applicable in this context. Otherwise LGTM
The full spec is available here - unfortunately only available to Microsoft employees currently.
Microsoft Reviewers: Open in CodeFlow