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

Am I the only finding rule no-loc-expr-outside-params extremely irritating #6210

Closed
slavizh opened this issue Mar 15, 2022 · 11 comments
Closed
Assignees
Labels
devdiv Related to Bicep tooling efforts in DevDiv Needs: Author Feedback Awaiting feedback from the author of the issue Needs: Upvote This issue requires more votes to be considered
Milestone

Comments

@slavizh
Copy link
Contributor

slavizh commented Mar 15, 2022

Would you consider that this rule is disabled by default? It is best practice that your resource is in the same location as your resource group. Of course there are exception but in general resource group and resource should be in the same one. The rule appears not only when

location: resourceGroup().location

but also when resourceGroup().location is being used in conditions, variables etc. For me the rule first does not work correctly but also should not be enabled by default as it is the opposite of best practice.

@ghost ghost added the Needs: Triage 🔍 label Mar 15, 2022
@majastrz majastrz added Needs: Author Feedback Awaiting feedback from the author of the issue Needs: Upvote This issue requires more votes to be considered and removed Needs: Triage 🔍 labels Mar 16, 2022
@majastrz
Copy link
Member

Depending on the patterns in your templates, it seems the users fall into two camps on this one. Reusable modules likely will require ability to override location and not take a dependency on resourceGroup().location, so it's a good practice there. However, if you aren't publishing such modules or have clean isolation between regions, the rule is definitely irritating.

@majastrz
Copy link
Member

@slavizh do you think it'll be enough to make this easier for users who want to use resourceGroup().location if we make it a one-click to disable the rule at the scope of the workspace or project?

Separately, when you say "does not work correctly", is it just that you disagree with the practice the rule recommends or is there an actual bug in the rule? What would you expect there?

@slavizh
Copy link
Contributor Author

slavizh commented Mar 17, 2022

@majastrz it will make it a little bit easier although as mentioned I do not think the rule is that good to be enabled by default.

Also the rule does not work correctly as basically everywhere where you use resrouceGroup().location you get the warning even when you use the function on condition for resource like if (resrouceGroup().location =~ myResource.location).

At least make it to work only if you have something like:

location: resrouceGroup().location 

if you have some other cases like below warning should not appear.

location: empty(location) ? resrouceGroup().location : location
properties: {
  someProperty: resrouceGroup().location
}
resource trafficManager 'Microsoft.Network/trafficManagerProfiles@2018-08-01' existing = if (resourceGroup().location =~ location) {
  name: trafficManagerName
}

@slavizh
Copy link
Contributor Author

slavizh commented Mar 17, 2022

also I would guess more users are using Bicep to build their own templates than to build modules for publishing.

@Jaykul
Copy link
Contributor

Jaykul commented Jun 24, 2022

I think the rule only really makes sense on reusable modules.

The vast majority of our bicep files are intended as templates for deploying resource groups -- and are always run with the target scope as the resource group with New-AzResourceGroupDeployment ... and on top of that, the rule got added after we had a significant code base built up, and basically causes us to add configs to disable the rule in each repository except the one repository where we keep our modules.

At least in our world, the number of repositories where I want to disable this is far greater than the number where I want to enforce it.

@afscrome
Copy link
Contributor

I am firmly in the camp that this rule (and no-hardcoded-location) are too opinionated to be turned on by default.

I personally have had to disable these rules in EVERY project I've worked on - our convention is one resource group per region.
If anything I'd want a rule that complains whenever anything other than resourceGroup().location or 'Global' is used as a location as this would have been left on almost all the time.

  1. I've been introducing a number of new engineers to bicep recently, and the rule has presented an early hurdle for these engineers to overcome. I to start off by introducing the simplest thing you could possibly deploy - a single resource. Unfortunately a few lines in you hit your first warning. This forces me to either introduce variables earlier than I wanted to, or to tell them to ignore it and start teaching bad habits about warnings being ignorable.

  2. Another use case where I find the rule gets in the way is data sovereignty. Company / Legal policy may restrict me to deploying certain resources in exactly one region. In this case I want to NOT have the region as a parameter as that would allow for a potential breach in policy.

I can see the value in parameterising a location when writing a module you're sharing a module widely, (e.g. registry, template spec), but in every other cases forcing location to be variablised / parameterised feels like a premature optimisation

This reminds me of difference in .Net guidance around ConfigureAwait(false). In library code, you NEARLY ALWAYS want to use ConfigureAwait(false) . But in Application, you ALMOST NEVER want to use ConfigureAwait(false). (source: https://devblogs.microsoft.com/dotnet/configureawait-faq/)

These location rules feel similar, if writing code to publish to a registry, or share in other ways, you should ALMOST ALWAYS make location a parameter and not hardcoded as that means the module can be used by any user, regardless of their opinions on how to manage resource locations.

For Application code, there is no explicit need for this. A developer could chose to follow the opinionated approach of always parameterising location, or the should be free not to. Currently it is forced on them by it being on by default.

I'd guess the vast majority of Bicep development is for Application code rather than Library code, so it feels like having these rules on by default is optimising for the 10% case rather than the majority case. So I'd want to see these rules turned off by default, or at least set to info level as there are enough scenarios that these rules aren't needed I don't think it warrants being highlighted as a warning.

@J0F3
Copy link

J0F3 commented Jun 30, 2022

I think I am more on the side that the rule makes senses and should be keep enabled by default.

Is it not the reusability the main reason of doing IaC and automation? So IMO the rule makes not only sense for modules but also for single deployments. Even when writing a template which ist not used as module I think it makes sense to define a (single) location parameter which is set by default to resourceGroup().location and is then used through out the template. Because it gives you exactly the reusability in comparison by defining the location at different location, maybe even in different ways, throughout the template.

as it is the opposite of best practice.

@slavizh Why do you think that it is opposite of best practices? Also for ARM the recommendation exists already: https://docs.microsoft.com/en-us/azure/azure-resource-manager/templates/best-practices#location-recommendations-for-parameters. So it is actually nothing new in Bicep. Or are you referencing any other best practices. Just out of curiosity...

@slavizh
Copy link
Contributor Author

slavizh commented Jun 30, 2022

@J0F3 Best practice is that the resources in a resource groups should match in location. So if resource group is in West Europe the VM in that resource group should also be West Europe. So the rule opposes that practices by warning you you should not use resourceGroup().location on location property of resource. On top of that the rule is made that way that no matter where you use *().location function you get a warning with the exception of using it in default value in parameter. If you do simple deployments where you pass each property as separate parameter that is may be fine but we do not pass such parameters. We pass whole objects or arrays.

@J0F3
Copy link

J0F3 commented Jun 30, 2022

Ah ok, got you. Yea, in that context I agree with you that the rule is confusing.

On top of that the rule is made that way that no matter where you use *().location function you get a warning with the exception of using it in default value in parameter.

Here I am also with you. Maybe the rule should more check if the function is used directly on a single resource instead of a variable, parameter or a value from an object or array (which was passed to a parameter).
In any case it should allow you to use the location function for a variable or a condition.

@slavizh
Copy link
Contributor Author

slavizh commented Jul 1, 2022

yep. Even if you use location for something else like:

var foo = uniqueString(resourceGroup().location, 'foo')

you get the warning.

The rule just covers (forces you to write your code in) specific pattern which to my opinion is very simple and it is good to start learning bicep but when you start to write bicep in more advanced way it does not work.

@StephenWeatherford StephenWeatherford changed the title Am I the only finding rule no-loc-expr-outside-params for extremely irritating Am I the only finding rule no-loc-expr-outside-params extremely irritating Nov 1, 2022
@StephenWeatherford StephenWeatherford added the devdiv Related to Bicep tooling efforts in DevDiv label Nov 1, 2022
@StephenWeatherford StephenWeatherford moved this to Todo in Bicep Nov 1, 2022
@StephenWeatherford StephenWeatherford added this to the v0.13 milestone Nov 1, 2022
@StephenWeatherford StephenWeatherford self-assigned this Nov 1, 2022
@StephenWeatherford
Copy link
Contributor

@slavizh Okay, Stan, you've convinced me, the location rules will be turned off by default. :-). See #8013 (comment)

If it would be helpful to have a different rule available for your usage (e.g. maybe one that checks that all location properties are set to the same value, whether "location" or resourceGroup().location), please enter a new issue. Thanks!

Closing this one.

Repository owner moved this from Todo to Done in Bicep Nov 1, 2022
@StephenWeatherford StephenWeatherford modified the milestones: v0.13, v0.12 Nov 1, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
devdiv Related to Bicep tooling efforts in DevDiv Needs: Author Feedback Awaiting feedback from the author of the issue Needs: Upvote This issue requires more votes to be considered
Projects
Archived in project
Development

No branches or pull requests

6 participants