Skip to content
This repository has been archived by the owner on Nov 1, 2023. It is now read-only.

Add support for feature flags #2620

Merged
merged 14 commits into from
Dec 5, 2022

Conversation

tevoinea
Copy link
Member

@tevoinea tevoinea commented Nov 14, 2022

Summary of the Pull Request

What is this about?

Adds a new App Configuration resource to the onefuzz deploy.

Info on Pull Request

What does this include?

  • Deploy new App Config resource
  • Wire up the azure functions to allow using it
  • Demo 1 case where it can be used

Validation Steps Performed

How does someone test & validate?

  • Ran deploy locally
  • Enabled feature flag in azure
  • Validated it was enabled
  • Disabled feature flag in azure
  • Validated it was disabled

Ran check-pr

resource featureFlags 'Microsoft.AppConfiguration/configurationStores@2022-05-01' = {
name: appConfigName
location: location
sku:{
Copy link
Contributor

@stishkin stishkin Nov 14, 2022

Choose a reason for hiding this comment

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

Use managed identity instead of connection strings

App Configuration Data Reader which has GUID: 516239f1-63e1-4d78-a4de-a74fb236a071

resource featureFlags 'Microsoft.AppConfiguration/configurationStores@2022-05-01' = {
name: appConfigName
location: location
sku:{
Copy link
Contributor

Choose a reason for hiding this comment

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

resource func 'Microsoft.Web/sites@2021-03-01' existing = {
name:
}
var readerRoleId = '516239f1-63e1-4d78-a4de-a74fb236a071'

resource assignReaderRole 'Microsoft.Authorization/roleAssignments@2022-04-01' = {
name: guid('${func.identity.principalId}-role-assignment-reader')
scope: featureFlags
properties: {
principalId: func.identity.principalId
roleDefinitionId: subscriptionResourceId('Microsoft.Authorization/roleDefinitions', readerRoleId)
}
}

@stishkin
Copy link
Contributor

Deploying new azure resource just to store one value.... seems like too much ?
Are there any other configuration options we can move there as well ?

@codecov-commenter
Copy link

codecov-commenter commented Nov 14, 2022

Codecov Report

Merging #2620 (e0bfb20) into main (af806a3) will decrease coverage by 0.00%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##             main    #2620      +/-   ##
==========================================
- Coverage   29.29%   29.28%   -0.01%     
==========================================
  Files         289      289              
  Lines       35755    35765      +10     
==========================================
  Hits        10474    10474              
- Misses      25281    25291      +10     
Impacted Files Coverage Δ
...piService/ApiService/Functions/AgentCanSchedule.cs 0.00% <ø> (ø)
src/ApiService/ApiService/ServiceConfiguration.cs 0.00% <0.00%> (ø)
...ApiService/ApiService/onefuzzlib/OnefuzzContext.cs 0.00% <0.00%> (ø)
...vice/onefuzzlib/notifications/NotificationsBase.cs 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@tevoinea tevoinea marked this pull request as ready for review November 17, 2022 14:31
@tevoinea
Copy link
Member Author

Deploying new azure resource just to store one value.... seems like too much ? Are there any other configuration options we can move there as well ?

So far, I only added the one value to demonstrate how it would be used. This should be the way we do feature management going forward and also in a follow up PR I'm going to add the node dismissal strategy

@tevoinea tevoinea requested review from Porges and stishkin November 17, 2022 15:41
Co-authored-by: George Pollard <porges@porg.es>
@tevoinea tevoinea mentioned this pull request Nov 25, 2022
3 tasks
@tevoinea tevoinea merged commit 5936010 into microsoft:main Dec 5, 2022
@mgreisen mgreisen mentioned this pull request Dec 9, 2022
@ghost ghost locked as resolved and limited conversation to collaborators Jan 4, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants