Return configuration changes to caller #21

ChristineWanjau · 2025-10-30T13:19:13Z

This change removes the concept of dry-run from the JSImporter and introduces a new approach for exposing configuration changes. Instead of handling the printing of diffs internally, the importer now simply returns the configuration diff, leaving it to the caller to decide how to use it (e.g., print to console or process further).

The previous design tightly coupled the importer with output behavior (printing diffs), which limited flexibility. By returning the diff directly, portal, pipeline tasks or other external callers can decide on how to use the diff.

Copilot

Pull Request Overview

This PR refactors the import logic to distinguish between added, modified, and deleted configuration settings, and introduces a new ConfigurationDiff interface that is returned when the dryRun parameter is enabled.

Key changes:

Introduces ConfigurationDiff interface with Added, Modified, and Deleted arrays
Refactors import logic into analyzeConfigurationChanges method to separate analysis from execution
Modified settings are now tracked separately from additions

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

File	Description
libraries/azure-app-configuration-importer/src/models.ts	Adds `ConfigurationDiff` interface and imports required types from @azure/app-configuration
libraries/azure-app-configuration-importer/src/index.ts	Exports the new `ConfigurationDiff` interface
libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts	Refactors import logic to distinguish between added/modified/deleted settings and returns `ConfigurationDiff` when dryRun=true

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

Copilot · 2025-11-03T06:28:08Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+        if (!settingsAreEqual) {
+          configurationSettingToModify.push(incoming);
+          // Remove from add list since it's a modification, not an addition
+          const addIndex: number = configurationSettingToAdd.findIndex(addSetting => 
+            addSetting.key === incoming.key && addSetting.label === incoming.label);
+          if (addIndex !== -1) {
+            configurationSettingToAdd.splice(addIndex, 1);
+          }


Inefficient array operations: for each existing setting that differs, the code performs a findIndex followed by splice on the configurationSettingToAdd array. With N incoming settings and M existing settings that differ, this results in O(N*M) complexity. Consider using a Set or Map data structure to track which settings should be added vs modified, or remove from the add list during the initial loop rather than searching for it later.

Copilot · 2025-11-03T06:28:08Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+        else if (importMode == ImportMode.IgnoreMatch) {
+          // Remove unchanged settings from add list
+          const addIndex = configurationSettingToAdd.findIndex(addSetting => 
+            addSetting.key === incoming.key && addSetting.label === incoming.label);
+          if (addIndex !== -1) {
+            configurationSettingToAdd.splice(addIndex, 1);
          }


Similar performance issue: this block also performs findIndex and splice operations within a loop. This should be optimized along with the modification logic in lines 123-127.

Copilot

Pull Request Overview

Copilot reviewed 5 out of 5 changed files in this pull request and generated 9 comments.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

libraries/azure-app-configuration-importer/tests/appConfigurationImporter.spec.ts

zhenlan · 2025-11-07T19:09:03Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+   * @param customHeadersOption - Custom headers for the operation.
+   * @returns ConfigurationChanges object containing Added, Modified, and Deleted settings
+   */
+  public async getConfigurationChanges(


Should the first letter be upper case given this is a public function?

Yes, updated thank you

zhenlan · 2025-11-07T19:12:54Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+    configSettingsSource: ConfigurationSettingsSource,
+    strict = false,
+    importMode?: ImportMode,
+    customHeadersOption?: OperationOptions


How is this used in this function?

The customHeadersOptions?

Right. It does look like needed for this function.

We use customHeadersOption to pass the correlationRequestId to link requests involved in the Import process. When the Import API calls the GetConfigurationChanges API, we pass the same correlationRequestId to associate both the read and write operations.
Additionally, we could use this to link the call to request the diff and Import on the portal for better traceability.

Discussed offline. Updated to generate correlationrequestid if not passed to GetConfigurationChanges

MaryanneNjeri · 2025-11-19T18:05:40Z

libraries/azure-app-configuration-importer/src/models.ts

  }
+}
+
+export interface ConfigurationChanges {


If a config hasn't change, do we also return it as part of the ConfigurationChanges

Only when import mode is equal to ALL; if a config hasn't changed, then it will be part of the changes

MaryanneNjeri · 2025-11-19T18:17:45Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

    timeout: number,
    strict = false,
    progressCallback?: (progress: ImportProgress) => unknown,
+    importMode?: ImportMode


We can set a defaultValue for importMode

public async Import( configSettingsSource: ConfigurationSettingsSource, timeout: number, strict = false, progressCallback?: (progress: ImportProgress) => unknown, importMode = ImportMode.IgnoreMatch

Much cleaner - Updated

RichardChen820 · 2025-11-20T07:20:35Z

libraries/azure-app-configuration-importer-file-source/package.json

  "author": "Microsoft Corporation",
  "description": "A client library for importing/exporting key-values between configuration file sources and Azure App Configuration service",
-  "version": "1.1.3-preview",
+  "version": "2.1.3-preview",


Should be 2.0.0-preview

MaryanneNjeri · 2025-11-20T08:10:07Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+  }
+
+  /**
+   * Get configuration changes between source settings and Azure App Configuration service without any applying changes


What does it mean "without any applying changes" ? What do you think of "Get configuration changes between source settings and existing settings in Azure App Configuration service...`

I meant - "without applying any changes". Don't you think its beneficial to mention this so that users can understand they can call it safely just to preview changes?

Suggesting -"Get configuration changes between source settings and existing settings in Azure App Configuration service without applying any changes"

MaryanneNjeri · 2025-11-20T08:12:03Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+      importMode = ImportMode.IgnoreMatch;
+    }
+
+    this.validateImportMode(importMode);


Looks like we will validateImportMode twice in Import function and in function to GetConfigurationChanges

We should still validateImportMode in GetConfigurationChanges since it might be called independently

But since we call GetConfigurationChanges in Import, and in GetConfigurationChanges we validateImportMode, do we need to validateImportMode again in Import ?

RichardChen820 · 2025-11-20T08:25:34Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+
+    this.validateImportMode(importMode);
+
+    // Generate correlation ID for operations


// Generate correlationRequestId for operations in the same activity

MaryanneNjeri · 2025-11-20T08:28:37Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+        if (!settingsAreEqual) {
+          configurationSettingToModify.push(incoming);
+          // Remove from add list since it's a modification, not an addition
+          const addIndex = configurationSettingToAdd.indexOf(incoming);


We can simplify this to configurationSettingsToAdd.splice(configurationSettingToAdd.indexOf(incoming), 1).

RichardChen820 · 2025-11-20T08:36:06Z

libraries/azure-app-configuration-importer/src/models.ts

+  Deleted: ConfigurationSetting<string>[];
+  Modified: SetConfigurationSettingParam<string | FeatureFlagValue | SecretReferenceValue>[];
+  Added: SetConfigurationSettingParam<string | FeatureFlagValue | SecretReferenceValue>[];


How about rename these fields to ToDelete ToModify, ToAdd? The current names seem to imply that the change has been made.

Makes sense to me. Updated

MaryanneNjeri · 2025-11-20T08:48:40Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+        configurationSettingToDelete.push(existing);
+      }
+
+      const incoming = configSettings.find(configSetting => configSetting.key == existing.key && 


I'd like to suggest instead of adding settings then removing them, maybe we shouldn't add them in the first place, maybe we can have something like this

if (importMode == ImportMode.IgnoreMatch) { const incoming = configSettings.find(configSetting => configSetting.key == existing.key && configSetting.label == existing.label); if (!incoming) { configSettingToAdd.push(existing); } else if (incoming && !isConfigSettingEqual(incoming, existing)) { configSettingToModify.push(existing); } }

Doesn't have to be exactly like this just an idea

RichardChen820 · 2025-11-20T08:48:43Z

libraries/azure-app-configuration-importer/src/appConfigurationImporter.ts

+      }
+    };
+
+    const configurationChanges: ConfigurationChanges = await this.GetConfigurationChanges(configSettingsSource, strict, importMode, customHeadersOption);


I'm trying to imagine, how the GetConfigurationChanges() and Import() will be used at client side. One possible situation is, client calls GetConfigurationChanges to preview what are the settings to be changed, then it may ask user 's promotion to apply the change or not, if user confirm to apply the change by calling Import(), In this case, I think Import() should no longer need to call the GetConfigurationChanges() again, just apply the chanages directly.

So seems we can't just blindly call GetConfigurationChanges in Import, Import should have another overload that can accept ConfigurationChanges as an argument.

Thoughts? @MaryanneNjeri @ChristineWanjau

@RichardChen820 I agree, and Import would no longer need to take parameters like strict or import mode ?

When I think of the current design, the caller would call GetConfigurationChanges twice, once when they call GetConfigurationChanges and second time when they want to Import.

This is a good point. This is the most likely flow in the client side especially portal. We are planning to preview the changes to the caller then they can click on apply immediately after.

Just one concern I have is consistency issues, what if the pre-calculated changes may no longer be accurate?

Right, it completely does not make sense, and also have risk that GetConfigurationChanges may return different values in these two attempts

Since we provide a preview, only performing the changes exactly as shown in the preview can be considered consistent.

This leads to a result where we delete an item that the customer was never told would be deleted. This is the real inconsistency from the customer’s point of view

I see, this makes sense.
Considering we won't delete anything added into the store before import happens, to some extent there is not much risk involved.

Just confirm so we have a second overload of the Import() function that takes in ConfigurationChanges.

public async Import( configSettingsSource: ConfigurationSettingsSource, timeout: number, strict?: boolean, progressCallback?: (progress: ImportProgress) => unknown, importMode?: ImportMode ): Promise<void>; /** * Import pre-calculated configuration changes into the Azure App Configuration service * * * @param configurationChanges - Pre-calculated changes from GetConfigurationChanges() * @param timeout * @param progressCallback * @returns Promise<void> */ public async Import( configurationChanges: ConfigurationChanges, timeout: number, progressCallback?: (progress: ImportProgress) => unknown ): Promise<void>;

Great! I have updated the function

cwanjau added 2 commits October 30, 2025 16:16

Return configuration diff when dry-run is true

8e39079

Fix lint issues

ff919a4

ChristineWanjau marked this pull request as draft October 30, 2025 13:30

Fix failing tests

bde8c64

ChristineWanjau marked this pull request as ready for review November 3, 2025 06:25

ChristineWanjau requested a review from Copilot November 3, 2025 06:25

Copilot AI reviewed Nov 3, 2025

View reviewed changes

cwanjau added 3 commits November 3, 2025 09:29

Rename function

5640e71

Add new api to return diff to caller

3dec8f0

Update variable name

d0f60b6

ChristineWanjau changed the title ~~Return configuration diff when dry-run is true~~ Return configuration changes to caller Nov 7, 2025

ChristineWanjau requested a review from Copilot November 7, 2025 08:10

Fix linting issues

0c44519

Copilot AI reviewed Nov 7, 2025

View reviewed changes

zhenlan reviewed Nov 7, 2025

View reviewed changes

cwanjau added 3 commits November 10, 2025 10:39

Address PR comments

be23383

Address PR comments

4a70857

address comments

8d5f1cd

MaryanneNjeri reviewed Nov 19, 2025

View reviewed changes

RichardChen820 reviewed Nov 20, 2025

View reviewed changes

MaryanneNjeri reviewed Nov 20, 2025

View reviewed changes

address comments

04e88e4

RichardChen820 reviewed Nov 20, 2025

View reviewed changes

MaryanneNjeri reviewed Nov 20, 2025

View reviewed changes

RichardChen820 reviewed Nov 20, 2025

View reviewed changes

Address comments

3c436af

MaryanneNjeri reviewed Nov 20, 2025

View reviewed changes

RichardChen820 reviewed Nov 20, 2025

View reviewed changes

cwanjau added 3 commits November 21, 2025 12:52

Add Import overload

bc799ad

Fix linting error

0b15321

Add tests

5f9c46a


		this.validateImportMode(importMode);

		// Generate correlation ID for operations

Return configuration changes to caller #21

Are you sure you want to change the base?

Return configuration changes to caller #21

Uh oh!

Conversation

ChristineWanjau commented Oct 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Nov 3, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristineWanjau Nov 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristineWanjau Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

MaryanneNjeri Nov 19, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ChristineWanjau Nov 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

ChristineWanjau commented Oct 30, 2025 •

edited

Loading

ChristineWanjau Nov 12, 2025 •

edited

Loading

ChristineWanjau Nov 19, 2025 •

edited

Loading

MaryanneNjeri Nov 19, 2025 •

edited

Loading

ChristineWanjau Nov 20, 2025 •

edited

Loading

RichardChen820 Nov 20, 2025 •

edited

Loading

ChristineWanjau Nov 20, 2025 •

edited

Loading

ChristineWanjau Nov 20, 2025 •

edited

Loading