Port TTK rule: artifacts-parameter.test.ps1

[StephenWeatherford]

TTK sources: https://github.com/Azure/arm-ttk/blob/master/arm-ttk/testcases/deploymentTemplate/artifacts-parameter.test.ps1
TTK unit tests: https://github.com/Azure/arm-ttk/tree/master/unit-tests/artifacts-parameter
Old docs: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-test-cases#artifacts-parameter-defined-correctly

[StephenWeatherford]

@bmoore-msft Would like to verify and understand how _artifactsLocation* parameters function in the bicep world. Will this continue to be a valid and compelling scenario? Thanks.

[bmoore-msft]

Same as in the JSON world - for marshalling any non-template artifacts... Scripts, binaries, zip, dsc config, sql bacpac, etc...

[ChristopherGLewis]

Can I ask why this exists??? There's no reason for the "artifacts" parameters to be formatted according to this TTK rule. It certainly makes them pretty, but in the bicep world, we're moving away from params that start with an _.

In fact, these parameters could be called 'Fred' and 'Sally' for all that bicep/arm cares.

If you go down this path, you'll have linter rules mandating that param names match the fields they are setting (vmSKU vs vmSize, Name vs DiskName) which is a rabbit hole you don't want to go down.

[StephenWeatherford]

cc @bmoore-msft

@ChristopherGLewis
There are tools that look for these parameters with this specific naming. So this is a very specific scenario.

Do you use those parameters without the underscore? Is it useful to be able to keep the rest of the rule but turn off that part?

[bmoore-msft]

Can I ask why this exists??? There's no reason for the "artifacts" parameters to be formatted according to this TTK rule. It certainly makes them pretty, but in the bicep world, we're moving away from params that start with an _.
In fact, these parameters could be called 'Fred' and 'Sally' for all that bicep/arm cares.

The rule follows a pattern that's been in ARM deployments since the beginning - not everyone follows the pattern and the rule can be turned off..

If you go down this path, you'll have linter rules mandating that param names match the fields they are setting (vmSKU vs vmSize, Name vs DiskName) which is a rabbit hole you don't want to go down.

Aside from location I haven't seen this develop over the years - so far the ecosystem seems to be limiting it to where it makes sense.

[ChristopherGLewis]

Yes, but is it a good pattern? Just because it was always this way doesn't mean it's right. I really dislike that in a VM deployment with many parameters, these two are "suggested" to start with an underscore.

[bmoore-msft]

I don't know if it's good or bad, but it hasn't been a problem to date and changing/breaking at this point could be bad. Though I'm not sure I understand the concern....

If you don't use parameters with the distinct naming pattern, then the rule doesn't apply. If you do use parameters with that distinct naming pattern, then you've opted into the rule. If you use that pattern for naming parameters and use those parameters for some other purpose the rule can be disabled.

What's the behavior you're expecting?

[ChristopherGLewis]

I'm expecting BICEP not to care what I name parameters, other than those outside Bicep's best practices.

Bicep recommends camel-case, and these Arti's don't match Bicep best practices. Now there's a linter rule saying that these (and only these) should start with "" - EVEN THOUGH naming them anything else doesn't break ARM/BICEP nor the deployment.

This just seems arbitrary, and not within the norm for ARM/Bicep. I hate "Always do X, EXCEPT for" when the "except for" has no reason other than inertia.

[bmoore-msft]

I'm expecting BICEP not to care what I name parameters, other than those outside Bicep's best practices.

I think this is why we have the ability to turn off rules... i.e. if you don't agree with a defined best practice...

[StephenWeatherford]

@ChristopherGLewis Do you actually use artifactsParameters as a parameter name? And if so, are the other parts of the rule helpful? I.e., is there any disadvantage for you turning off the rule completely? Or does it just bother you?

There is also the question of if the rule is on or off by default, but unless this detrimentally affects a lot of people, I'd prefer it be on by default. Thanks.

[ChristopherGLewis]

I'm expecting BICEP not to care what I name parameters, other than those outside Bicep's best practices.

I think this is why we have the ability to turn off rules... i.e. if you don't agree with a defined best practice...

Which I have. but that's for my instance of vscode via my bicepconfig.json.

How do I get this to apply to the module so it doesn't apply if someone else uses the template? is there a #pragma line equivalent in bicep or is the only way to turn off the linter line is via the bicepconfig.json file - which applies to everything rather than the one file I want it to.

[ChristopherGLewis]

@ChristopherGLewis Do you actually use artifactsParameters as a parameter name? And if so, are the other parts of the rule helpful? I.e., is there any disadvantage for you turning off the rule completely? Or does it just bother you?

There is also the question of if the rule is on or off by default, but unless this detrimentally affects a lot of people, I'd prefer it be on by default. Thanks.

I use artifactsLocation and artifactsLocationSasToken rather than _artifactsLocation and _artifactsLocationSasToken and my templates have work perfectly fine for thousands of VMs. Again, this rule has no place in reality, it's just arbitrary.

Your statement:

There are tools that look for these parameters with this specific naming. So this is a very specific scenario.

is confusing for me. If Bicep/ARM/Azure all support any parameter name for these, what tools are you indicating? And shouldn't ARM/Azure be the determining factor of what's allowed?

There's no disadvantage to turning off the rule completely, there's just no scoping nuance to the bicepconfig.json file. And bicepconfig.json is separate from the ARM/Bicep - there's no way to ignore the rule for just this bicep template.

I guess my biggest issue is that this rule got turned on with the latest version of Bicep. I'm concerned that if we have linter rule defaults that change over time there will be pipelines/build processes that break when new versions of bicep go out. I know we're still in the "breaking changes are acceptable" phase, but this is a bad precedence to set.

[StephenWeatherford]

@ChristopherGLewis

There are tools that look for these parameters with this specific naming. So this is a very specific scenario.

is confusing for me. If Bicep/ARM/Azure all support any parameter name for these, what tools are you indicating?

At least three places I know of where these parameters have expected names:

1. The _artifactsParameters usage was started by Visual Studio, whose ARM tools support deployment that would automatically fill in these parameter values as part of build/deploy (in order to make deploying nested templates easier - they were set I believe as the location of where a nested template was temporarily staged in storage so that the published template that used it would have a definitive location to look to. The temporary staging was set up automatically as part of the process.)
2. I believe it's used similarly in Azure QuickStarts Templates (e.g. https://github.com/Azure/azure-quickstart-templates/blob/master/Deploy-AzureResourceGroup.ps1)
3. I'm told Azure marketplace uses it
   Likely others. Doesn't mean other uses aren't valid, although the rule was built with this in mind (original here: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/template-test-cases#artifacts-parameter-defined-correctly)

And shouldn't ARM/Azure be the determining factor of what's allowed?

This doesn't limit what's allowed, it's a warning for a particular best practice usage as Brian noted, the aptness of which can certainly be discussed.

There's no disadvantage to turning off the rule completely, there's just no scoping nuance to the bicepconfig.json file. And bicepconfig.json is separate from the ARM/Bicep - there's no way to ignore the rule for just this bicep template.

To answer your questions on this: You can have multiple levels of bicepconfig.json files to scope a rule's settings to a particular directory (i.e. have a bicepconfig.json in the folder with that bicep file, and just mention the one rule to turn off). You can also turn it off for a particular line with:
#disable-next-line artifacts-parameters
We plan on adding a "#disable-file" soon as well.

Also, the expectation is that the bicepconfig.json file is checked in, it's not intended to be per-user.

I guess my biggest issue is that this rule got turned on with the latest version of Bicep. I'm concerned that if we have linter rule defaults that change over time there will be pipelines/build processes that break when new versions of bicep go out. I know we're still in the "breaking changes are acceptable" phase, but this is a bad precedence to set.

We're starting to think about this, but it didn't make a lot of sense until there were a reasonable number of rules. Here's a first proposal that deals with defaults of rules (up until now we've mostly turned them all on by default to get better usage feedback): #8013. Haven't decided yet on a versioning scheme to keep existing pipelines from breaking, although as long as you don't upgrade bicep the rules won't currently change on you. Feel free to create an issue to discuss.

POSSIBLE SOLUTIONS:
As for the immediate issue, these are the possibilities I see:

1. Leave things as they are, require people to disable if they use artifactsParameters without the underscore (and hence without the tooling needs)
2. Remove the part of the rule that requires underscores, and only checks the other requirements when it finds parameters with underscore)
3. Remove the part of the rule that requires underscores, but have the other requirements checked for both artifactsParameters/etc and _artifactsParameters/etc, whichever is found
4. Change the default of the rule to "off" (could be combined with some of the above)

My current inclination is 2 or 4

cc @ucheNkadiCode

[ucheNkadiCode]

I think the main issue here is an issue with how far to go on "reserving" a param name. As Brian mentioned, there is some tooling and paradigms connected to how _artifactsLocation and _artifactsLocationSasToken are used.

There are some names of params in Bicep we've already "reserved" as special such as param location. As an example, if you say

param location = "westus"

we've put tooling around the fact that the param is named location and you've given a specific value for it. In this case, we are adding tooling around _artifactsLocation and _artifactsLocationSasToken in line with the ARM-ttk.

@StephenWeatherford and @bmoore-msft, I would like to ensure this rule is not too rigid. So if someone even creates param _artifactsLocation1 then all of the rules should not apply. I also do not want this rule to expand to if a user only writes param artifactsLocation (without the underscore).

Let's stick to the heart of the ttk rule and ONLY invoke those type/default value rules if the user uses _artifactsLocation and _artifactsLocationSasToken.

This should provide a healthy compromise to @ChristopherGLewis feedback. I do apologize if you happen to be using _artifactsLocation or _artifactsLocationSasToken in your current Bicep files. While we understand your feedback, there are other folks who still use the ARM-ttk and it is our goal to provide them an easy transition to Bicep.

[StephenWeatherford]

@ucheNkadiCode
#8016 - remove check for underscores as decided

@ChristopherGLewis if you haven't, you might want to read my responses above about limiting the scope of a linter rule. I'd also be interested in how we could make those features better known (something in the docs maybe?). If you're using the vscode editor the lightbulb next to the error offers the disable-next-line solution as well as editing the bicepconfig.json (although doesn't mention anything about multiple levels of config).

Please continue to provide feedback, we are very interested in providing features that work well for everyone, so we need to also know when things aren't working out.

[ChristopherGLewis]

@ucheNkadiCode #8016 - remove check for underscores as decided

@ChristopherGLewis if you haven't, you might want to read my responses above about limiting the scope of a linter rule. I'd also be interested in how we could make those features better known (something in the docs maybe?). If you're using the vscode editor the lightbulb next to the error offers the disable-next-line solution as well as editing the bicepconfig.json (although doesn't mention anything about multiple levels of config).

Please continue to provide feedback, we are very interested in providing features that work well for everyone, so we need to also know when things aren't working out.

Thanks for the responses to this - I believe that this destined outcome operates much better in my opinion.

I didn't know about this line:

#disable-next-line artifacts-parameters

I like to be up to speed on all things bicep, but I missed that one, which is exactly the solution I needed.

I will note that my lightbulb does not always show #disable-next-line artifacts-parameters as an option, but only when the cursor is in the squiggly yellow line word. When it's in the param or at the start, you only see @ decorators.

vs

which might be a little confusing since the position of the lightbulb doesn't change.

Note also that when you mouse over the error the main error window shows "No quick fixes available":

[ucheNkadiCode]

Wow @ChristopherGLewis! That is a very interesting Bug! I'll make sure to add it to our backlog. Thank you for pointing that out. I wonder if this extends to many more disable-next-line suggestions

In terms of the error with the quick fix, I think we simply didn't implement logic to take care of that use case. However #8016 should take care of this issue since we will no longer worry about "ArtifactsLocation" or "artifactslocation" whatsoever. Only if there is an underscore that precedes it. I still mentioned this issue in #8023