-
Notifications
You must be signed in to change notification settings - Fork 1
Update feature flag equality check #17
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 pull request updates the feature flag equality check to use the MsFeatureFlagValue type and to extend the equality comparison to additional properties.
- Update type usage from FeatureFlagValue to MsFeatureFlagValue.
- Augment the equality check with extra comparisons for allocation, variants, and telemetry.
- Refactor array comparison by introducing a generic helper function for comparing arrays of client filters and variants.
Reviewed Changes
| File | Description |
|---|---|
| libraries/azure-app-configuration-importer/src/internal/utils.ts | Updated equality check and type handling for feature flag values |
Copilot reviewed 1 out of 1 changed files in this pull request and generated no comments.
Comments suppressed due to low confidence (3)
libraries/azure-app-configuration-importer/src/internal/utils.ts:166
- [nitpick] The aliasing of the generic array comparator as areFeatureFlagFiltersEqual (and similarly for variants) may reduce clarity. Consider renaming these helper functions to more clearly reflect their specific purposes if their intended use cases differ.
const areFeatureFlagFiltersEqual = areFeatureFlagArraysEqual<FeatureFlagClientFilters>;
libraries/azure-app-configuration-importer/src/internal/utils.ts:148
- Confirm that 'isEqual' performs a deep equality check for 'allocation' as intended, particularly if these properties can contain nested or complex data structures.
isEqual(featureFlagAValue.allocation, featureFlagBValue.allocation) &&
libraries/azure-app-configuration-importer/src/internal/utils.ts:169
- [nitpick] Consider adding validation for the new properties ('allocation', 'variants', and 'telemetry') obtained from the parsed JSON to ensure their structures meet expectations before using them.
function toFeatureFlagValue(value: string): MsFeatureFlagValue {
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 updates the feature flag equality check by replacing the old FeatureFlagValue type with the new MsFeatureFlagValue type and adds extra comparisons for variant properties as well as allocation and telemetry.
- Updated type annotations in the isFeatureFlagValueEqual function to use MsFeatureFlagValue
- Introduced generic equality checks for arrays to compare client filters and variants
- Extended the toFeatureFlagValue function to parse additional properties such as allocation, variants, and telemetry
Reviewed Changes
| File | Description |
|---|---|
| libraries/azure-app-configuration-importer/src/internal/utils.ts | Updated feature flag equality logic and added support for variant, allocation, and telemetry properties |
Copilot reviewed 1 out of 1 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:166
- [nitpick] The aliasing of the generic 'areFeatureFlagArraysEqual' function as 'areFeatureFlagFiltersEqual' may not be immediately clear. Consider adding a brief comment to explain why the alias exists for improved readability.
const areFeatureFlagFiltersEqual = areFeatureFlagArraysEqual<FeatureFlagClientFilters>;
libraries/azure-app-configuration-importer/src/internal/utils.ts:148
- New equality checks for 'allocation', 'variants', and 'telemetry' have been added. Please ensure that there are corresponding unit tests that validate these new comparison scenarios.
isEqual(featureFlagA.allocation, featureFlagB.allocation) &&
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
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 updates the feature flag equality check to include variant properties and additional fields like telemetry and allocation. Key changes include:
- Updated equality logic in the utility functions to handle MsFeatureFlagValue.
- Added new test cases to verify equality and inequality for feature flags with variant properties.
- Introduced a generic helper function (areArrayEqual) for comparing arrays for filters and variants.
Reviewed Changes
| File | Description |
|---|---|
| libraries/azure-app-configuration-importer/tests/util.spec.ts | Added test cases for feature flag equality including variant properties |
| libraries/azure-app-configuration-importer/src/internal/utils.ts | Updated utility functions to use MsFeatureFlagValue and included comparison for allocation, variants, and telemetry |
Copilot reviewed 2 out of 2 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:150
- The telemetry field is now compared in the equality check but there is no test case covering scenarios where telemetry is present on one side and absent on the other. Consider adding tests to confirm this behavior.
isEqual(featureFlagAValue.telemetry, featureFlagBValue.telemetry);
libraries/azure-app-configuration-importer/tests/util.spec.ts:501
- The extra property 'status_override' in one of the variant objects in testKeyValue10 may cause the equality comparison to fail due to a mismatch in the variant objects. Confirm that this behavior is intended or adjust the conversion logic accordingly.
"status_override":"None"
This PR adds the variant properties in the feature flag equality check.