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

Example of Condition deploy logging resource for Web App #1199

Merged
merged 7 commits into from Dec 21, 2020

Conversation

JFolberth
Copy link
Contributor

Tried new condition functionality to see how it would work. Figured commit it back for others.

@codecov-io
Copy link

codecov-io commented Dec 20, 2020

Codecov Report

Merging #1199 (1771b1b) into main (469cce3) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             main    #1199   +/-   ##
=======================================
  Coverage   94.49%   94.49%           
=======================================
  Files         336      336           
  Lines       16825    16825           
  Branches       14       14           
=======================================
  Hits        15898    15898           
  Misses        927      927           
Flag Coverage Δ
dotnet 95.03% <ø> (ø)
typescript 27.20% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

}
}

resource appServiceAppSettings 'Microsoft.Web/sites/config@2020-06-01' = if (logging) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

since the if (logging) condition is used multiple times, it may make sense to put all of those resources in a module and then conditionally deploy the module. That is a personal preference, so I will leave that to you

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense and can see that viewpoint. I think part of the question is overall how modules should be scoped. So far I've taken it from the individual resources as opposed to in this case the feature set of logging. Think realistically for this one a single logging module makes more sense.

Thanks for the feedback! I will update accordingly.

Copy link
Collaborator

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One comment. I will leave this here for the day to give you time to address the comment (or decide not to do it, which is fine)

Copy link
Collaborator

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the conditions in logging.bicep are redundant and can be removed

var appInsightName = toLower('appi-${appName}')
var logAnalyticsName = toLower('la-${appName}')

resource appServiceAppSettings 'Microsoft.Web/sites/config@2020-06-01' = if (logging) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think you need the if (logging) checks in the module, since the whole module will not be called unless the condition of the module is met in main.bicep. So you should be able to remove the logging param entirely

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wondered this and actually did put it at the module level....just left it inside. I did remove it and retest deployment. Got the same result.

Code changes pushed up.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the help!

Copy link
Collaborator

@alex-frankel alex-frankel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great! Love seeing the use of conditions

@alex-frankel alex-frankel merged commit 5c772ef into Azure:main Dec 21, 2020
@JFolberth JFolberth deleted the webapplogcondition branch December 21, 2020 20:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants