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

Allow referencing existing resources with the 'existing' keyword #1339

Merged
merged 14 commits into from
Feb 2, 2021

Conversation

anthony-c-martin
Copy link
Member

@anthony-c-martin anthony-c-martin commented Jan 19, 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

Adds the ability to reference runtime properties from an existing resource which is not being deployed by the Bicep file.

Fixes #258

@anthony-c-martin anthony-c-martin force-pushed the antmarti/external_resources branch 5 times, most recently from 71205a8 to b0dcdcc Compare January 26, 2021 10:43
@anthony-c-martin anthony-c-martin force-pushed the antmarti/external_resources branch from b0dcdcc to 71c8301 Compare January 27, 2021 03:01
@miqm
Copy link
Collaborator

miqm commented Jan 27, 2021

I think more "natural" syntax would be existing resource res 'Provider/Type@version' = { } or even just existing res 'Provider/Type@version' = { } (without resource keyword)

@anthony-c-martin anthony-c-martin force-pushed the antmarti/external_resources branch from 71c8301 to 8bb2b40 Compare January 27, 2021 12:28
@anthony-c-martin
Copy link
Member Author

anthony-c-martin commented Jan 27, 2021

I think more "natural" syntax would be existing resource res 'Provider/Type@version' = { } or even just existing res 'Provider/Type@version' = { } (without resource keyword)

Please comment on #258 for syntax discussion - this PR is just to implement the design we closed on. I do think there's scope for changing this and definitely share some of your thoughts here, but it'll have to come in as a later revision.

@anthony-c-martin anthony-c-martin force-pushed the antmarti/external_resources branch from 9daa5ed to cc171f5 Compare January 27, 2021 16:16
@anthony-c-martin anthony-c-martin changed the title WIP: 'existing' keyword Allow referencing existing resources with the 'existing' keyword Jan 27, 2021
@codecov-io
Copy link

codecov-io commented Jan 28, 2021

Codecov Report

Merging #1339 (5fd94b5) into main (37a4cce) will decrease coverage by 0.07%.
The diff coverage is 92.53%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1339      +/-   ##
==========================================
- Coverage   94.80%   94.73%   -0.08%     
==========================================
  Files         351      352       +1     
  Lines       18348    18494     +146     
  Branches       14       14              
==========================================
+ Hits        17395    17520     +125     
- Misses        953      974      +21     
Flag Coverage Δ
dotnet 95.23% <92.53%> (-0.08%) ⬇️
typescript 27.20% <ø> (ø)

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

Impacted Files Coverage Δ
...Core.IntegrationTests/Emit/TemplateEmitterTests.cs 100.00% <ø> (ø)
...cep.Core/TypeSystem/Az/ManagementGroupScopeType.cs 80.00% <0.00%> (-20.00%) ⬇️
...Bicep.Core/TypeSystem/Az/ResourceGroupScopeType.cs 80.00% <0.00%> (-20.00%) ⬇️
.../Bicep.Core/TypeSystem/Az/SubscriptionScopeType.cs 80.00% <0.00%> (-20.00%) ⬇️
src/Bicep.Core/TypeSystem/Az/TenantScopeType.cs 60.00% <0.00%> (-20.00%) ⬇️
src/Bicep.LangServer/SemanticTokenVisitor.cs 0.00% <0.00%> (ø)
src/Bicep.Core/Syntax/SyntaxRewriteVisitor.cs 71.42% <40.00%> (+0.11%) ⬆️
src/Bicep.Core/TypeSystem/ResourceScopeType.cs 60.00% <50.00%> (ø)
src/Bicep.Core/TypeSystem/ModuleType.cs 87.50% <66.66%> (-12.50%) ⬇️
src/Bicep.Core/TypeSystem/ResourceType.cs 90.00% <66.66%> (-10.00%) ⬇️
... and 39 more

@anthony-c-martin anthony-c-martin marked this pull request as ready for review January 28, 2021 16:13
@anthony-c-martin anthony-c-martin force-pushed the antmarti/external_resources branch from 7b5a72d to f27c21f Compare January 29, 2021 16:44
Copy link
Collaborator

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

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

Thanks for adding the doc update. I can update https://github.com/Azure/bicep/blob/main/docs/arm2bicep.md when this gets checked in

docs/spec/resources.md Outdated Show resolved Hide resolved
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:

Copy link
Contributor

@shenglol shenglol left a comment

Choose a reason for hiding this comment

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

Looks great!

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.

Support "external" resource references that are not declared in the template
6 participants