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

Fixes for nested resource and scope interop issues #1827

Merged
merged 3 commits into from
Mar 11, 2021

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Mar 11, 2021

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
  • 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

Fixes #1810

This fixes the following issues:

  1. Setting 'scope' should not be permitted on nested child resources
  2. It should not be possible to deploy a nested child for an existing resource at a different scope to that of the template
  3. Scope not being accounted for when generating resourceIds for extension resources
  4. Scope note being accounted for when generating 'existing' resource references
  5. Simplify formatting of nested resource reference names (avoid concat + split when we already have a list of names)

@codecov-io
Copy link

codecov-io commented Mar 11, 2021

Codecov Report

Merging #1827 (ac9730c) into main (fb1a053) will increase coverage by 0.04%.
The diff coverage is 99.12%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1827      +/-   ##
==========================================
+ Coverage   95.03%   95.07%   +0.04%     
==========================================
  Files         371      371              
  Lines       21671    21882     +211     
  Branches       15       15              
==========================================
+ Hits        20594    20805     +211     
  Misses       1077     1077              
Flag Coverage Δ
dotnet 95.51% <99.12%> (+0.04%) ⬆️
typescript 26.61% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
src/Bicep.Core/Diagnostics/Diagnostic.cs 100.00% <ø> (ø)
src/Bicep.Core/Semantics/Symbol.cs 85.71% <ø> (ø)
src/Bicep.Core/Emit/ResourceDependencyVisitor.cs 91.25% <88.88%> (-2.18%) ⬇️
src/Bicep.Core.IntegrationTests/DecoratorTests.cs 100.00% <100.00%> (ø)
...Core.IntegrationTests/Emit/TemplateEmitterTests.cs 100.00% <100.00%> (ø)
...Bicep.Core.IntegrationTests/NestedResourceTests.cs 100.00% <100.00%> (ø)
src/Bicep.Core.IntegrationTests/ScenarioTests.cs 100.00% <100.00%> (ø)
src/Bicep.Core.IntegrationTests/ScopeTests.cs 100.00% <100.00%> (ø)
...icep.Core.UnitTests/Assertions/JTokenAssertions.cs 100.00% <100.00%> (ø)
...UnitTests/Assertions/JTokenAssertionsExtensions.cs 95.65% <100.00%> (+3.65%) ⬆️
... and 7 more

},
"dependsOn": [
"[resourceId('My.Rp/parentType/childType', split(format('{0}/{1}', 'basicParent', 'basicChild'), '/')[0], split(format('{0}/{1}', 'basicParent', 'basicChild'), '/')[1])]",
"[resourceId('My.Rp/parentType/childType', 'basicParent', 'basicChild')]",
Copy link
Member

Choose a reason for hiding this comment

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

basicParent [](start = 52, length = 11)

This was annoying me when I was converting some of our internal templates to Bicep.

@@ -13,7 +13,7 @@ namespace Bicep.Core.UnitTests.Assertions
{
public static class JTokenAssertionsExtensions
{
public static JTokenAssertions Should(this JToken instance)
public static JTokenAssertions Should(this JToken? instance)
Copy link
Member

Choose a reason for hiding this comment

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

JToken [](start = 51, length = 6)

I'm guessing this led to usages of ! to suppress the nullness error all over the place. Worth going back to clean up?

Copy link
Member

@majastrz majastrz left a comment

Choose a reason for hiding this comment

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

:shipit:

@anthony-c-martin anthony-c-martin enabled auto-merge (squash) March 11, 2021 20:54
@anthony-c-martin anthony-c-martin merged commit 225441a into main Mar 11, 2021
@anthony-c-martin anthony-c-martin deleted the antmarti/nestedfixes branch March 11, 2021 21:09
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.

Nested resource syntax missing validation and generates invalid code when used with 'scope'
3 participants