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 foundation for extension config assignment syntax in bicepparam files and extensionConfigs on module declarations #16565

Open
wants to merge 16 commits into
base: main
Choose a base branch
from

Conversation

kalbert312
Copy link
Member

@kalbert312 kalbert312 commented Mar 7, 2025

Contributing a Pull Request

If you haven't already, read the full contribution guide. The guide may have changed since the last time you read it, so please double-check. Once you are done and ready to submit your PR, run through the relevant checklist below.

Contributing a feature

  • I have opened a new issue for the proposal, or commented on an existing one, and ensured that the Bicep maintainers are good with the design of the feature being implemented: Foundation for Integration of extension resources with deployment stacks. bicep-reps#6

  • I have included "Fixes #{issue_number}" in the PR description, so GitHub can link to the issue and close it when the PR is merged

  • I have appropriate test coverage of my new feature

  • I have a resource name property equal to "name"

  • If applicable, I have set the location property to location: /*${<id>:location}*/'location' (not resourceGroup().location) where <id> is a placeholder id, and added param location string to the test's main.bicep file so that the resulting main.combined.bicep file used in the tests compiles without errors

  • I have verified that the snippet deploys correctly when used in the context of an actual bicep file

    e.g.

    resource aksCluster 'Microsoft.ContainerService/managedClusters@2021-03-01' = {
      name: 'name'
Microsoft Reviewers: Open in CodeFlow

Choose a reason for hiding this comment

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

PR Overview

This PR adds foundational support for extension config assignment syntax while enhancing test coverage and updating related diagnostics.

  • Adds new integration tests for module extension configurations.
  • Updates baseline samples and CLI integration tests for improved error assertions.
  • Refactors module diagnostics and template generation methods in integration tests.

Reviewed Changes

File Description
src/Bicep.Core.IntegrationTests/ExtensibilityTests.cs Introduces tests for valid and invalid extension config assignments.
src/Bicep.Core.Samples/DataSets.cs Adds a new DataSet sample ("Extensions_CRLF") for extension-related scenarios.
src/Bicep.Cli.IntegrationTests/BuildParamsCommandTests.cs Adds additional non-null assertions and updates expected diagnostic validations.
src/Bicep.Core.Samples/BaselineData_Bicepparam.cs Updates the baseline data list to include extension-related parameters files.
src/Bicep.Core.IntegrationTests/ModuleTests.cs Refactors diagnostic retrieval and template generation to use updated API methods.

Copilot reviewed 69 out of 69 changed files in this pull request and generated no comments.

Comments suppressed due to low confidence (1)

src/Bicep.Core.Samples/DataSets.cs:21

  • [nitpick] The naming 'Extensions_CRLF' could be more descriptive; consider adding a comment to clarify its intent (e.g., indicating that it targets tests involving CRLF line endings) or renaming it for better clarity.
public static DataSet Extensions_CRLF => CreateDataSet();
name: 'moduleWithExtsWithAliases'
//@ "name": "moduleWithExtsWithAliases",
extensionConfigs: {
//@ "extensionConfigs": {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not certain why this is separate from the rest.

"contentVersion": "1.0.0.0",
"parameters": {},
"extensionConfigs": {
"k8s": {}
Copy link
Member Author

Choose a reason for hiding this comment

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

Property emission not yet implemented.

Copy link
Contributor

github-actions bot commented Mar 7, 2025

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

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

Copy link
Contributor

github-actions bot commented Mar 7, 2025

Dotnet Test Results

    78 files   -     39      78 suites   - 39   31m 17s ⏱️ - 15m 31s
11 996 tests +    47  11 996 ✅ +    47  0 💤 ±0  0 ❌ ±0 
27 750 runs   - 13 690  27 750 ✅  - 13 690  0 💤 ±0  0 ❌ ±0 

Results for commit 4cefd1b. ± Comparison against base commit 450ba67.

This pull request removes 1792 and adds 671 tests. Note that renamed tests count towards both.

		nestedProp1: 1
		nestedProp2: 2
		prop1: true
		prop2: false
	1
	2
	\$'")
	prop1: true
	prop2: false
…
Bicep.Cli.IntegrationTests.BuildCommandTests ‑ Build_Valid_SingleFile_WithTemplateSpecReference_ShouldSucceed_Extensions_CRLF
Bicep.Cli.IntegrationTests.BuildCommandTests ‑ Build_Valid_SingleFile_WithTemplateSpecReference_ToStdOut_ShouldSucceed_Extensions_CRLF
Bicep.Cli.IntegrationTests.BuildParamsCommandTests ‑ Build_Valid_Params_File_Should_Succeed (Files/baselines_bicepparam/Extensions/parameters.bicepparam)
Bicep.Cli.IntegrationTests.BuildParamsCommandTests ‑ Build_Valid_Params_File_ToStdOut_Should_Succeed (Files/baselines_bicepparam/Extensions/parameters.bicepparam)
Bicep.Cli.IntegrationTests.BuildParamsCommandTests ‑ Build_Valid_Params_File_To_Outdir_Should_Succeed (Files/baselines_bicepparam/Extensions/parameters.bicepparam)
Bicep.Cli.IntegrationTests.FormatCommandTests ‑ Format_SampleBicepFile_MatchesFormattedSample_Extensions_CRLF
Bicep.Cli.IntegrationTests.FormatCommandTests ‑ Format_SampleBicepParam_MatchesFormattedSample (Files/baselines_bicepparam/Extensions/parameters.bicepparam)
Bicep.Cli.IntegrationTests.PublishCommandTests ‑ Extensions_CRLF, with docUri, publishing source
Bicep.Cli.IntegrationTests.PublishCommandTests ‑ Extensions_CRLF, without docUri, not publishing source
Bicep.Cli.IntegrationTests.PublishCommandTests ‑ Publish_ValidArmTemplateFile_AllValidDataSets_ShouldSucceed_Extensions_CRLF
…

♻️ This comment has been updated with latest results.

@kalbert312 kalbert312 changed the title Add foundation for extension config assignment syntax. Add foundation for extension config assignment syntax in bicepparam files and extensionConfigs on module declarations Mar 7, 2025
@@ -0,0 +1,7 @@
using 'main.bicep'

extension k8s with {
Copy link
Member Author

Choose a reason for hiding this comment

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

Not sure why the symbol isn't showing up.

@@ -184,6 +184,9 @@ public LanguageExpression ConvertExpression(Expression expression)
case ParametersAssignmentReferenceExpression exp:
return CreateFunction("parameters", new JTokenExpression(exp.Parameter.Name));

case ExtensionConfigAssignmentExpression exp:
Copy link
Member Author

@kalbert312 kalbert312 Mar 7, 2025

Choose a reason for hiding this comment

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

I'm not sure if this is needed. This syntax is only usable in param files. I think referencing assignments from other assignments should be permissible, but that's for a later PR.

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.

1 participant