-
Notifications
You must be signed in to change notification settings - Fork 742
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 resource types in outputs and parameters #4971
Conversation
ad7d66d
to
1b55383
Compare
src/Bicep.Core.Samples/Files/InvalidParameters_LF/main.diagnostics.bicep
Outdated
Show resolved
Hide resolved
"type": "string", | ||
"metadata": { | ||
"resourceType": "Microsoft.Storage/storageAccounts@2019-06-01" | ||
} |
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.
Worth discussion on the best way to encode this. We need the type to round-trip through modules. The way I encoded it we always include the API Version when we write, but when we read we ignore it for type validation purposes.
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.
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.
We should probably continue the discussions on this part after this PR goes in.
First off, this is very cool. Some things I noticed when testing:
I have a module I wonder if it is related to this being a module collection resulting from a loop expression? |
Something that we could consider as a follow-up - a function to get the scope of the resource. This way we could say to module - deploy with a scope of this particular resource. |
Just realized this is probably the same issue as #4343 |
We shouldn't necessarily need a function to get the scope if I am understanding the scenario correctly. We need to support a generic resource param without a specific type so that the following scenario of creating an extension resource for several different types can work: param genericResource resource
resource diagnostic 'Microsoft.Insights/diagnosticSettings@2021-05-01-preview' = {
scope: genericResource
...
} EDIT: Looks this is currently blocked, but I think we need this to work: |
We should catch up on this today. When @anthony-c-martin was explaining to me the set of validations that take place for scoping, my reaction was that we can't really enforce those with any amount of certainty. Maybe I'm wrong about this, or maybe the feature is still valuable without the validation 😆 |
Currently the type is required for all outputs. So if it's an int then it's |
Maybe not. I told the editor to colorize this as a keyword, which sounds like it's not what we want. https://github.com/Azure/bicep/pull/4971/files#diff-ee8a096413276bc466bb160724239726958a72cb0c16dd566bb268dacfbdafe0R264 |
Let's discuss this today. After we planned to block this use case the generic 'any resource' type didn't really make sense so we didn't implement it. If we need to his this use case then we need to bring these back into the design. |
Use case scenario for this function would be to have a module that adds secrets to given key vault or sets up key vault secrets user role. Key vault might be in a different RG (scope) than module that deploys i.e. WebApp. We need to set module scope so the deployment goes where it should, hence the need for a function to extract scope from resourceId. |
There was also idea to just mark resource as 'public' and use the symbol name but that could give extra complication with nested ones, so perhaps let's stick with output. |
There are a few scenarios where specifying the type is required, without a way around it - so we cannot infer it completely:
|
Fixing this. |
695fcce
to
e25bd60
Compare
subscribing, to do some more testing once these artifacts are generated. |
db3e4f2
to
7dbf395
Compare
Is this being worked on? Can't wait to get this functionality... |
9c41db6
to
bb74d77
Compare
I pushed an update here. This doesn't yet address all of the scenarios that we've discussed (PR description is up to date). The plan I discussed with the maintainers is to merge this as an experimental feature first and then address the gaps in a series of smaller changes. |
src/Bicep.Decompiler/Rewriters/ParentChildResourceNameRewriter.cs
Outdated
Show resolved
Hide resolved
// We need to validate all of the parameters and outputs to make sure they are valid types. | ||
// This is where we surface errors for 'unknown' resource types. | ||
if (singleDeclaredType is ModuleType moduleType && | ||
moduleType.Body is ObjectType objectType) |
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.
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.
Are you sure? This is looking at types not syntax. L281 ^^^ handles the array case.
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 know there's still work left but this is awesome! Since this is behind a feature flag, all the comments I added can be addressed in future PRs. |
c8b74a3
to
81f8e28
Compare
Fixes: Azure#2246 This change implements functionality for declaring strongly type parameters and outputs using resource types. Example: ``` param storage resource 'Microsoft.Storage/storageAccounts@2020-01-01' ``` This declares a parameter that can be interacted with as-if it were an 'existing' resource type declaration of the provided type. In addition you can do the same with outputs: ``` output out resource 'Microsoft.Storage/storageAccounts@2020-01-01' = foo ``` or using type inference (outputs): ``` output out resource = foo ``` These features together allow you to pass resources across module boundaries, and as command line parameters (using the resource ID). --- This PR implements Azure#2246 with a few caveats that I discussed offline with one of the maintainers. 1. It does not include the 'any resource type' that's outlined in the proposal. It's not clear how that part of the proposal will work in the future with extensibility. 2. It does not support extensibility resources currently. To do this we need to define the semantics of how an extensibility resource can be serialized (what's the equivalent of `.id`). This is explicitly blocked in the first cut and can be added as extensibility is designed. 3. Parameter resources cannot be used with the `.parent` or `.scope` properties because it would allow you to bypass scope validation easily. The set of cases that we could actually provide validation for these use cases are really limited.
81f8e28
to
bd456e7
Compare
Fixes: #2246 This change implements functionality for declaring strongly type parameters and outputs using resource types. Example: ``` param storage resource 'Microsoft.Storage/storageAccounts@2020-01-01' ``` This declares a parameter that can be interacted with as-if it were an 'existing' resource type declaration of the provided type. In addition you can do the same with outputs: ``` output out resource 'Microsoft.Storage/storageAccounts@2020-01-01' = foo ``` or using type inference (outputs): ``` output out resource = foo ``` These features together allow you to pass resources across module boundaries, and as command line parameters (using the resource ID). --- This PR implements #2246 with a few caveats that I discussed offline with one of the maintainers. 1. It does not include the 'any resource type' that's outlined in the proposal. It's not clear how that part of the proposal will work in the future with extensibility. 2. It does not support extensibility resources currently. To do this we need to define the semantics of how an extensibility resource can be serialized (what's the equivalent of `.id`). This is explicitly blocked in the first cut and can be added as extensibility is designed. 3. Parameter resources cannot be used with the `.parent` or `.scope` properties because it would allow you to bypass scope validation easily. The set of cases that we could actually provide validation for these use cases are really limited.
Fixes: #2246
This change implements functionality for declaring strongly type
parameters and outputs using resource types.
Example:
This declares a parameter that can be interacted with as-if it were an
'existing' resource type declaration of the provided type.
In addition you can do the same with outputs:
or using type inference (outputs):
These features together allow you to pass resources across module
boundaries, and as command line parameters (using the resource ID).
This PR implements #2246 with a few caveats that I discussed offline
with one of the maintainers.
proposal. It's not clear how that part of the proposal will work
in the future with extensibility.
need to define the semantics of how an extensibility resource can
be serialized (what's the equivalent of
.id
). This is explicitlyblocked in the first cut and can be added as extensibility is
designed.
.parent
or.scope
properties because it would allow you to bypass scope validation
easily. The set of cases that we could actually provide validation
for these use cases are really limited.
Contributing a feature