Skip to content

Use schema $ref resolution from ARM template engine in ArmTemplateSemanticModel#9621

Merged
jeskew merged 5 commits intomainfrom
jeskew/$ref-fixup
Jan 23, 2023
Merged

Use schema $ref resolution from ARM template engine in ArmTemplateSemanticModel#9621
jeskew merged 5 commits intomainfrom
jeskew/$ref-fixup

Conversation

@jeskew
Copy link
Copy Markdown
Contributor

@jeskew jeskew commented Jan 23, 2023

This PR spins off a small refactor from #9454, which is blocked pending the ARM w02 rollout.

Microsoft Reviewers: Open in CodeFlow

Co-authored-by: Anthony Martin <38542602+anthony-c-martin@users.noreply.github.com>
public ErrorDiagnostic CyclicArmTypeRefs(IEnumerable<string> cycleLinks) => new(
public ErrorDiagnostic UnresolvableArmJsonType(string errorSource, string message) => new(
TextSpan,
"BCP312",
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We've so far tried to avoid re-using error codes, in case it makes errors more difficult to google - please could you pick a new one?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

The error code being replaced hasn't been included in any releases yet, but that makes a few assumptions about when this PR will be merged and whether we'll need to do a release before then. Updated to use BCP318 instead.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Oh my bad, I missed that. Thanks for addressing anyway!

catch (TemplateValidationException tve)
{
return ErrorType.Create(errorBuilder(DiagnosticBuilder.ForDocumentStart()));
return ErrorType.Create(DiagnosticBuilder.ForDocumentStart().UnresolvableArmJsonType(tve.TemplateErrorAdditionalInfo.Path ?? "<unknown location>", tve.Message));
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Please could you ensure we've got coverage to assert the error message is correctly formatted? A scenario test or baseline test would probably suffice.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

There is a test for this in ArmTemplateSemanticModelTests, but it didn't include any assertions about the error message or code. Updated that test to cover the message.

# Conflicts:
#	src/Bicep.Cli.IntegrationTests/packages.lock.json
#	src/Bicep.Cli.UnitTests/packages.lock.json
#	src/Bicep.Cli/packages.lock.json
#	src/Bicep.Core.IntegrationTests/packages.lock.json
#	src/Bicep.Core.Samples/packages.lock.json
#	src/Bicep.Core.UnitTests/packages.lock.json
#	src/Bicep.Decompiler.IntegrationTests/packages.lock.json
#	src/Bicep.Decompiler.UnitTests/packages.lock.json
#	src/Bicep.Decompiler/packages.lock.json
#	src/Bicep.LangServer.IntegrationTests/packages.lock.json
#	src/Bicep.LangServer.UnitTests/packages.lock.json
#	src/Bicep.LangServer/packages.lock.json
#	src/Bicep.RegistryModuleTool.IntegrationTests/packages.lock.json
#	src/Bicep.RegistryModuleTool.TestFixtures/packages.lock.json
#	src/Bicep.RegistryModuleTool.UnitTests/packages.lock.json
#	src/Bicep.RegistryModuleTool/packages.lock.json
#	src/Bicep.Tools.Benchmark/packages.lock.json
#	src/Bicep.Wasm/packages.lock.json
# Conflicts:
#	src/Bicep.Core/Diagnostics/DiagnosticBuilder.cs
@jeskew jeskew enabled auto-merge (squash) January 23, 2023 18:52
@jeskew jeskew merged commit 38f27a5 into main Jan 23, 2023
@jeskew jeskew deleted the jeskew/$ref-fixup branch January 23, 2023 19:24
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