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

Add 'localDeploy' experimental feature, base extension nuget library #14231

Merged
merged 2 commits into from
Jun 4, 2024

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Jun 3, 2024

  • Add wiring for "localDeploy" experimental feature. The feature doesn't do anything yet as of this PR.
  • Add "Bicep.Local.Extension" nuget package to contain logic for C# extension authoring.
  • Add "extension.proto" to defined the contract between client (Bicep) and extension in gRPC.
  • Support unpacking of provider packages to the local file system to the ~/.bicep cache folder.

The full spec is available here - unfortunately only available to Microsoft employees currently.

Microsoft Reviewers: Open in CodeFlow

Copy link
Contributor

github-actions bot commented Jun 3, 2024

Dotnet Test Results

    65 files   -     34      65 suites   - 34   22m 59s ⏱️ - 8m 39s
10 876 tests  -     20  10 875 ✅  -     20  1 💤 ±0  0 ❌ ±0 
25 648 runs   - 12 820  25 646 ✅  - 12 819  2 💤  - 1  0 ❌ ±0 

Results for commit a7998b2. ± Comparison against base commit ba1e9f8.

♻️ This comment has been updated with latest results.

Copy link
Contributor

github-actions bot commented Jun 3, 2024

Test this change out locally with the following install scripts (Action run 9369999284)

VSCode
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-vsix.sh) --run-id 9369999284
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-vsix.ps1) } -RunId 9369999284"
Azure CLI
  • Mac/Linux
    bash <(curl -Ls https://aka.ms/bicep/nightly-cli.sh) --run-id 9369999284
  • Windows
    iex "& { $(irm https://aka.ms/bicep/nightly-cli.ps1) } -RunId 9369999284"

Comment on lines -23 to +24
this.mainLayer = this.Layers.Single();
var expectedLayerMediaType = BicepMediaTypes.BicepProviderArtifactLayerV1TarGzip;
this.mainLayer = this.Layers.Where(l => l.MediaType.Equals(expectedLayerMediaType, MediaTypeComparison)).Single();
Copy link
Contributor

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?

Copy link
Member Author

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
Copy link
Contributor

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.

Copy link
Member Author

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.

Copy link
Contributor

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.

Bicep.sln Show resolved Hide resolved
Copy link
Contributor

@jeskew jeskew left a 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

@anthony-c-martin anthony-c-martin merged commit ded2a94 into main Jun 4, 2024
45 checks passed
@anthony-c-martin anthony-c-martin deleted the ant/localdeploy_1 branch June 4, 2024 15:32
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.

2 participants