-
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 experimental 'local-deploy' command group to Bicep CLI #14243
Conversation
5a30edf
to
d2ebeef
Compare
Test this change out locally with the following install scripts (Action run 9421684319) VSCode
Azure CLI
|
e3f9cb7
to
608232b
Compare
The VS test failure appears to be caused by microsoft/vstest-action#31 - not a problem with the PR. |
@@ -153,7 +153,7 @@ public static async Task PublishProviderToRegistryAsync(IDependencyHelper servic | |||
throw new InvalidOperationException($"Failed to get reference '{errorBuilder(DiagnosticBuilder.ForDocumentStart()).Message}'."); | |||
} | |||
|
|||
await dispatcher.PublishProvider(targetReference, new(tgzData)); | |||
await dispatcher.PublishProvider(targetReference, new(tgzData, false, [])); |
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 not using named args in bicep repo the norm? In this case I had to look into what argument - in PublishProvider -corresponds to a record and find its bool argument, which corresponds to LocalDeployEnabled
.
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.
Yeah, my personal style is to avoid named args by default. I will sometimes use them if I feel like they add clarity, but IMO they often get in the way of code readability without adding any useful context.
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.
IMHO I'd argue that having a naked "false" or "true" argument is much less readable. When possible I prefer to redesign the API (e.g. use an enum), but otherwise I like using named args or inline comments for this case.
Not that this is a big deal...
this.moduleDispatcher = moduleDispatcher; | ||
this.providerFactory = providerFactory; | ||
// Built in provider for handling nested deployments | ||
RegisteredProviders[new("LocalNested", "0.0.0")] = new AzExtensibilityProvider(this); |
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.
Don't recall if we have a way to load nested deployments provider version, I think it might be good to let users decide which version to use rather than picking a specific version?
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.
Bicep doesn't expose the nested deployment API version to end users in the module
syntax, so it's not possible to pick a different version. This is by design - Bicep should be in control of the behavior; if we need to opt-in to new behavior, then we will do so in a new Bicep release.
} | ||
|
||
return new( | ||
Types: dataDict["types.tgz"], |
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 types.tgz going to contain types + providers right?
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.
also, is it possible for types.tgz
to not be present in the dict and a key not found exception to be thrown?
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.
Yeah, the error handling here could use some work before we remove the "experimental" feature flag.
ee3d413
to
dac6641
Compare
3c86a4f
to
21eb401
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.
21eb401
to
df86f76
Compare
return null; | ||
} | ||
|
||
var data = BinaryData.FromStream(fileSystem.FileStream.New(PathHelper.ResolvePath(binaryPath), FileMode.Open, FileAccess.Read, FileShare.Read)); |
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.
"using" to close the stream?
|
||
this.EmitDependsOn(emitter, module.DependsOn); | ||
|
||
// Since we don't want to be mutating the body of the original ObjectSyntax, we create an placeholder body in place |
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.
nit
// Since we don't want to be mutating the body of the original ObjectSyntax, we create an placeholder body in place | |
// Since we don't want to be mutating the body of the original ObjectSyntax, we create a placeholder body in place |
{ | ||
if (SupportedArchitectures.TryGetCurrent() is not {} architecture) | ||
{ | ||
throw new InvalidOperationException($"Unsupported architecture: {RuntimeInformation.ProcessArchitecture}"); |
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.
Probably these error messages should be different, different causes.
@@ -0,0 +1,102 @@ | |||
// Copyright (c) Microsoft Corporation. |
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 didn't see this documented for --help. I assume that's intentional for now?
Addressing some comments that @StephenWeatherford raised in #14243. ###### Microsoft Reviewers: [Open in CodeFlow](https://microsoft.github.io/open-pr/?codeflow=https://github.com/Azure/bicep/pull/14252)
This change brings in the following:
local-deploy
command group to deploy a Bicep file using the local extensibility preview.publish-provider
command group to allow publishing a binary extension to an ACR or to file system.To test it out (see "Test this change out" comment below first):
bicepconfig.json
:main.bicep
:main.bicepparam
:Microsoft Reviewers: Open in CodeFlow