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

[BREAKING CHANGE PROPOSAL] new "outputs" syntax #1563

Closed
alex-frankel opened this issue Feb 17, 2021 · 28 comments · Fixed by #1623
Closed

[BREAKING CHANGE PROPOSAL] new "outputs" syntax #1563

alex-frankel opened this issue Feb 17, 2021 · 28 comments · Fixed by #1623
Assignees
Labels
enhancement New feature or request Needs: Author Feedback Awaiting feedback from the author of the issue
Milestone

Comments

@alex-frankel
Copy link
Collaborator

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.

// not valid since both symbols are called "foo"
param foo string
var foo = 123

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 a var (i.e. param.foo or var.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 variable storageAccountName stores a dynamic value that you'd like to output:

var storageAccountName = uniqueString(resourceGroup().id)
output storageAccountName string = storageAccountName // not valid

Proposed syntax change

As a result of this problem, we'd like to propose the following syntax change for output declaration:

output <EXPRESSION> as <TYPE> [<IDENTIFIER>]
var foo = 123
output foo as int

With the optional identifier, you would have

output 1 + 1 as int foo

Going back to our previous realistic example, that would change to:

var storageAccountName = uniqueString(resourceGroup().id)
output storageAccountName as string

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:

var foo = 42
output foo int = foo

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 var foo.

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.

@alex-frankel alex-frankel added the enhancement New feature or request label Feb 17, 2021
@ghost ghost added the Needs: Triage 🔍 label Feb 17, 2021
@alex-frankel alex-frankel added Needs: Author Feedback Awaiting feedback from the author of the issue and removed Needs: Triage 🔍 labels Feb 17, 2021
@alex-frankel alex-frankel changed the title [Breaking change] new "outputs" syntax [BREAKING CHANGE PROPOSAL] new "outputs" syntax Feb 17, 2021
@SimonWahlin
Copy link
Collaborator

I'm not a fan of the proposed syntax.

  1. This would be the only place where we put type before a name/identifier
  2. The 'as' keyword can be just as confusing or more as the circular dependency (for me it implies trying to cast to type)
  3. When trying the proposed syntax in my bicep files I find it harder to see all output names, sometimes the output name is implied by the expressions being a reference and sometimes it is renamed. Always having the output name in the same place makes it a lot easier to scan the code without spending unnecessary brain-cycles on parsing the line. It breaks the flow of the code and I don't like it.
var foo                = 42
var storageAccountName = uniqueString(resourceGroup().id)

output storageAccountName as string myStorageAccountName
output storageAccountName as string
output 1 + 1 as int two
output myVnet.subnets as array

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.

var foo                = 42
var storageAccountName = uniqueString(resourceGroup().id)

output myStorageAccountName string = storageAccountName
output storageAccountName string   = storageAccountName
output two int                     = 1 + 1
output subnets array               = myVnet.subnets

@ld0614
Copy link

ld0614 commented Feb 17, 2021

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 output storageAccountName string = storageAccountName

@slavizh
Copy link
Contributor

slavizh commented Feb 17, 2021

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.

@StefanIvemo
Copy link
Collaborator

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.

@takekazuomi
Copy link
Contributor

I think the ‘as’ syntax is simple enough and straightforward. The current output name collision problem is serious. Therefore, I choose the proposed syntax.

@sebader
Copy link
Member

sebader commented Feb 17, 2021

+1 to @SimonWahlin

The order of something like this just feels really weird to me output 1 + 1 as int foo

@PalmEmanuel
Copy link
Contributor

Agreeing with @SimonWahlin as well, having as in only the output feels clunky and out of place (I also connect the keyword mainly with type conversion from C# for example). I agree with the points he makes against the proposal.

@SebastianClaesson
Copy link

+1 for @SimonWahlin .
I feel the first option more confusing as it breaks how you for example write parameters.
The second option, defining outputs as

output two int = 1 + 1
output subnets array = myVnet.subnets

feels more consistent with how you are writing bicep templates today.

@miqm
Copy link
Collaborator

miqm commented Feb 17, 2021

After giving some though on pros and cons - 👍🏻 for @SimonWahlin and @bmoore-msft.

I think the only reason why the as syntax can be better is to avoid the clash of the names if we use the shortened version.

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. vars aren't variables but constants they do not change during runtime). In ARM we used variables so we would not need to build the resource name every time we reference it. Now in bicep I tend to define name directly in resource and then use myResource.name which makes using myResourceName obsolete. For child resources i still use vars, but with #127 and #1564 that will not be needed anymore.

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 output myResourceName string = myResourceName (which I don't find confusing at all - there are more confusing language syntaxes like in javascript 😉)

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.

@bmoore-msft
Copy link
Contributor

@miqm - well stated... re: this...

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.

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.

@majastrz
Copy link
Member

Output loops will look like the following with both syntaxes:

Current

output myOutput array = [for thing in things: <loop body>]

Proposed

output [for thing in things: <loop body>] as array

@J0F3
Copy link

J0F3 commented Feb 18, 2021

I agree with @SimonWahlin. I also like the alternative more. (For the same reasons as @SimonWahlin and @miqm set out above).

@miqm
Copy link
Collaborator

miqm commented Feb 18, 2021

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.

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) - last(split()) is a bit too extensive, now it's easier to output name.

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.

param existingResourceId string

resource existingResource from existingResourceId

resource existingResource:child 'subtype@api-version' = {
  name: 'childName'
}

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.

param existingResourceId string

resource existingResource 'Microsoft.Type/resource@version' existing from existingResourceId

resource existingResource:child 'subtype@version' = {
  name: 'childName'
}

@WhitWaldo
Copy link

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:

var two = 1 + 1 with output
param existingResourceId "abc123" with output

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.

@kilasuit
Copy link

as unfortunately blends in far to much, whereas a symbol like = breaks the line into chunks, which make it more easily scan read whilst ensuring the context of the line still stays with the reader.

I personally think the below works better for readability (variation on @SimonWahlin suggestion)

output string myStorageAccountName = storageAccountName
output string storageAccountName   = storageAccountName
output int two                     = 1 + 1
output array subnets               = myVnet.subnets

this feels more easily read as I have an output which is a string called myStorageAccountName
but that maybe because this is a common way to read in dotnet based languages

@miqm
Copy link
Collaborator

miqm commented Feb 18, 2021

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?

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.

@bmoore-msft
Copy link
Contributor

@majastrz in this example:

output [for thing in things: <loop body>] as array

what's the name of the output?

@majastrz
Copy link
Member

@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

@kilasuit
Copy link

@majastrz we already require an explicit name for every output & that should not change

@majastrz
Copy link
Member

@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.

@kilasuit
Copy link

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

@ChristopherGLewis
Copy link
Contributor

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

output 1 + 1 as int foo  <- Works
output {id = 1, value = 2} as object foo <- awkward 
output {id = 1,
            value1 = "a",
            value2 = "b",
            value2 = "c"} as object foo <- ugly

**type before value **

output foo as int = 1 + 1 <-  Works
output foo as object = {id = 1, value = 2} <- better

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
output foo = {id = 1, value = 2} <- inferred object
output foo = 1 + 1 <- inferred Int, but what about int64s ?
output foo as string = 1 + 1 <- Works, yielding a string "2"

@slapointe
Copy link
Contributor

I too prefer the second option discussed by @SimonWahlin. It help the code stay cleaner and easier to read.

@alex-frankel
Copy link
Collaborator Author

alex-frankel commented Feb 23, 2021

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.

@alex-frankel
Copy link
Collaborator Author

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 foo since we don't know which one is the "right" one. This is the same logic and UX we have for duplicate symbols in the global namespace.

@iamalexmang
Copy link

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 output foo int = foo (which I prefer) is similar to a declaration and initialization in almost any other programming language where the initialization commands the value of the local to take on the value of a parameter or class member.
Put differently, I prefer the latter option because its common to the syntax of other programming languages, feels more natural and follows a more intuitive approach. I could even see linting easily analyzing Bicep code and suggesting fixes to the output declaration and these would (or should) be easily understandable.

@miqm
Copy link
Collaborator

miqm commented Feb 25, 2021

@anthony-c-martin So which one is it finally?

@alex-frankel
Copy link
Collaborator Author

alex-frankel commented Feb 25, 2021

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.

@ghost ghost locked as resolved and limited conversation to collaborators May 28, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request Needs: Author Feedback Awaiting feedback from the author of the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.