-
Notifications
You must be signed in to change notification settings - Fork 1
Serialize feature flags before importing #16
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
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
PR Overview
This PR adds serialization for feature flags before their import to prevent variant properties from being dropped by the SDK.
- Introduces a new function to serialize feature flags into JSON strings.
- Updates imports and type definitions in utils.ts and models.ts to support serialization.
- Adjusts the default configuration settings converter to push serialized feature flag settings.
Reviewed Changes
| File | Description |
|---|---|
| libraries/azure-app-configuration-importer/src/internal/utils.ts | Adds the serializeFeatureFlagToConfigurationSettingParam function and updates imports to include featureFlagPrefix and JsonFeatureFlagValue. |
| libraries/azure-app-configuration-importer/src/models.ts | Adds the JsonFeatureFlagValue type definition. |
| libraries/azure-app-configuration-importer/src/internal/parsers/defaultConfigurationSettingsConverter.ts | Changes feature flag setting type to string and applies serialization before pushing to settings. |
Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (2)
libraries/azure-app-configuration-importer/src/internal/utils.ts:186
- [nitpick] Consider using an explicit method to remove the feature flag prefix instead of relying on String.replace, to ensure that only a single occurrence is removed if key contains multiple instances of the prefix.
id: featureFlag.value.id ?? key.replace(featureFlagPrefix, "")
libraries/azure-app-configuration-importer/src/internal/parsers/defaultConfigurationSettingsConverter.ts:258
- [nitpick] Consider updating the function or inline comments to indicate that the settings array now contains serialized feature flag values (strings) instead of raw MsFeatureFlagValue objects, to avoid potential misuse later in the code.
const serializedSetting: SetConfigurationSettingParam<string> = serializeFeatureFlagToConfigurationSettingParam(setting);
MaryanneNjeri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In FeatureFlagConfigurationSettingsConverter.Convert is the goal for value to be a string, so that this evaluates to true ?
...ure-app-configuration-importer/src/internal/parsers/defaultConfigurationSettingsConverter.ts
Outdated
Show resolved
Hide resolved
...ure-app-configuration-importer/src/internal/parsers/defaultConfigurationSettingsConverter.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
libraries/azure-app-configuration-importer/src/internal/utils.ts
Outdated
Show resolved
Hide resolved
Should we follow up with the SDK team? cc: @jimmyca15 |
I had opened an issue with the sdk team on this - Azure/azure-sdk-for-js#33266 |
This is a workaround since when importing variant feature flag with the current sdk the variant properties are dropped by the sdk here when it tries to serialize the feature flag here -https://github.com/Azure/azure-sdk-for-js/blob/87f78037cf2e562cba511e8699147b937c4a9ac7/sdk/appconfiguration/app-configuration/src/appConfigurationClient.ts#L233