-
Notifications
You must be signed in to change notification settings - Fork 772
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
[BREAKING CHANGE PROPOSAL] new "outputs" syntax #1563
Comments
I'm not a fan of the proposed syntax.
I do like the other option suggested, the only downside I can see is the potential confusion of an output name being identical to an identifier. I don't see that as a problem, output names are not identifiers and it should be very easy to learn. Also this suggestion gives me the option to use spaces to format output to align the equal signs for a very clean looking code.
|
I agree with @SimonWahlin that the alternative is a better option than the proposed change. I would argue that as output objects are always identified with the output syntax that it is clear (or at least easily taught) indicator of why |
I would go with the proposed syntax for the already stated reasons. Do not have strong opinion but if I have to choose the proposed syntax looks better to me. I am missing how this would look if you have copy/loop that outs array of objects. |
I also agree with @SimonWahlin here. I prefer the second option over the suggested syntax for the same reasons that Simon covers in his comment. |
I think the ‘as’ syntax is simple enough and straightforward. The current output name collision problem is serious. Therefore, I choose the proposed syntax. |
+1 to @SimonWahlin The order of something like this just feels really weird to me |
Agreeing with @SimonWahlin as well, having |
+1 for @SimonWahlin .
feels more consistent with how you are writing bicep templates today. |
After giving some though on pros and cons - 👍🏻 for @SimonWahlin and @bmoore-msft. I think the only reason why the However, as I looked at my bicep files currently in use, in opposite to the ARM, I don't use variables that often (n.b. That said, I think situations where we'll output a variable or parameter is going to be very rare and we can safely use the little 'confusing' syntax More often the case might be when where we'll name the output same as some existing resource for referencing it or using as parent in other module. However, considering #923 and #1170 we even might not need to do that explicitly as bicep will generate some technical outputs for us. |
@miqm - well stated... re: this...
More common (in bicep) will be outputting the resourceId in one file such that I can easily create a resource reference to it in the next... Which I think is to your overall point. |
Output loops will look like the following with both syntaxes: Currentoutput myOutput array = [for thing in things: <loop body>] Proposedoutput [for thing in things: <loop body>] as array |
I agree with @SimonWahlin. I also like the alternative more. (For the same reasons as @SimonWahlin and @miqm set out above). |
Agree, resourceId for references and that will be most common case in future. But now resourceName I use for parent/child relationship (i.e. you create KeyVault in top-level file and secrets with db passwords in db modules) - Speaking of working with resourceIds - would be great if we could convert string resourceId to a bicep resource so we can use it for parent/child relationships, e.g.
But not sure if this is possible in this way. I think we need to specify type regardless the fact that we will have it in resourceId string. But still at least name could be extracted by bicep.
|
Just jumping in with my two cents, but is there ever a situation where the output value isn't based on an expression assigned to a variable or parameter? Or could it be acceptable that outputs can only be set by a variable in Bicep? What if, instead of designating a whole line with the output keyword, we instead made this a decoration appended to the end of the declaration expression, as in:
The attached "with output" would indicate that two is used in the context of assigning a variable and also should be emitted as part of the output. |
I personally think the below works better for readability (variation on @SimonWahlin suggestion)
this feels more easily read as I have an output which is a string called myStorageAccountName |
I don't think so. it's pointless to engage nested deployment only to output what've input (with slight change). user defined functions will do this better and faster. outputs usually exposes values taken from resources that you create i.e. keys, identity ids, etc. |
@majastrz in this example:
what's the name of the output? |
@bmoore-msft We could choose something based on the expressions specified or always require an explicit name. With explicit names the proposed syntax would look like this: output [for thing in things: <loop body>] as array myOutput |
@majastrz we already require an explicit name for every output & that should not change |
@kilasuit the proposal, if implemented, in this issue would actually change it (the expression is used to determine the default name of the output), so my loops comment was inline with the proposal. I definitely agree it's a fairly big downside of the proposal to make output names less obvious/explicit. |
I've already suggested #1389 that would ensure ability for readable context - but also understand that it'll take you down the "too ARMy" path if you did that |
I would definitely support the syntax that @SimonWahlin proposes. As you start to get to more and more complex objects, the "value before type" syntax breaks: value before type
**type before value **
As for the global namespace - I'd suggest that all of Param, Var and Output into a single space is broken. Remember that Bicep is a simplification of ARM, but still represents ARM. While collapsing the Param and Var into a single namespace makes sense, adding output to that namespace shows issues like this. I would also ask that all outputs require a name and type, although type can be inferred |
I too prefer the second option discussed by @SimonWahlin. It help the code stay cleaner and easier to read. |
We discussed this yesterday, and based on all of the responses we have decided to go with Option 2, which effectively creates an "outputs" namespace that will not conflict with the other symbols in the same bicep file. This will no longer be a breaking change, but we will still try to get this done for our 0.3 release. Thank you all for jumping on this issue quickly and for articulating the concerns clearly. It made it really easy for us to discuss and revert our decision. I know I speak for the whole team in saying we are very thankful for how this community is operating! We will get the spec updated. |
Oh and I should also clarify that the same restrictions apply for all symbols in the outputs namespace. That means you still cannot have two outputs with the same name, for example: output foo int = 1
output foo int = 2 This will result in a red squiggle under both declarations of |
The problem at hand is serious enough to require it to be addressed asap. That said, the proposed workaround is okay, but far from ideal. In fact I like the alternative a hundred times better. This is probably my take on it looking at it from a programmer's perspective, where "as" implies casting, conversation as isn't typical for a declaration, whilst the |
@anthony-c-martin So which one is it finally? |
We are going to be implementing what @johndowns asked for in #569 (which is the proposal that @SimonWahlin pushed for in the initial response on this thread). Anthony has a PR out now: #1623. Closing this since we are no longer going to do the original breaking change proposal in this issue. |
Problem
Originally proposed and discussed in #569
Nearly all identifiers in bicep share a global namespace. This means I cannot declare a parameter with the same name as a variable, or a resource with the same name as an output, etc.
In most cases, this is actually a good thing for readability, otherwise we would need the referencing syntax to disambiguate whether the identifier is a
param
or avar
(i.e.param.foo
orvar.foo
). However,outputs
are unique because they are never referenced within the same bicep file, so it is overly limiting to not allow for duplicate identifiers in this case. Consider a more realistic example that is not possible today, where the variablestorageAccountName
stores a dynamic value that you'd like to output:Proposed syntax change
As a result of this problem, we'd like to propose the following syntax change for output declaration:
With the optional identifier, you would have
Going back to our previous realistic example, that would change to:
Other options explored
You can see a full accounting for all of the options we explored in #568, but the only other top contender was this option which aligns closer to the current syntax and would not be a breaking change:
While most familiar to the current
output
syntax, we feel it's too confusing since in the above case it looks like the output is assigning a value of itself, even though it is assigning the value of the varfoo
.ASK
We've already gotten some feedback in #569 that gives us confidence in our proposal, but we wanted to create this issue to make a more formal thread to give folks a final chance to review it. Please let us know if you see any potential issues with this new outputs syntax.
The text was updated successfully, but these errors were encountered: