add support for nested task groups #3007

Dev-Op · 2025-09-30T16:38:13Z

For our currently running migration with many classical (and somtime very old) pipelines I added some features to this project. The first is supporting task groups in task groups (nested task groups). With this change the task group migration run multiple times. In the first step all task groups with out task groups are migrated. In the second the task groups related on the migrated task groups. This will be done until no task group is missing or there was a problem.

Copilot

Pull Request Overview

This PR adds support for nested task groups in the Azure DevOps Pipeline Processor, enabling migration of task groups that contain other task groups. The implementation uses a multi-phase approach where unnested task groups are migrated first, followed by iterative processing of nested task groups until all dependencies are resolved.

Key changes:

Introduces multi-phase task group migration with dependency resolution
Adds service connection mapping support for task groups
Implements null-safe checks for task definition types

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

tomfrenzel

I like the idea of this PR 🙂

I'll take a more in depth look at the changes in the next days as I'm currently on vacation.

Until then, fix the comments Copilot made and try to get a successful build/test run. Copilot's requested changes look good to me. Additionally it would be nice if you introduce a helper function for this check:

t.Task.DefinitionType?.ToLower() == "metaTask".ToLower()

We call it many times and I think it makes sense that it is executed only in one single method.

…vOpsPipelineProcessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

add IsMetaTask method for multiple used checks

Copilot

Pull Request Overview

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

Copilot · 2025-10-17T19:08:47Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

+            do 
+            {
+                // We need to process the nested taskgroups in a loop, because they can contain other nested taskgroups.
+                taskGroupsToMigrate.Clear();
+                foreach (var taskGroup in nestedTaskGroups)
+                {
+                    var nestedTaskGroup = taskGroup.Tasks.Where(t => IsMetaTask(t.Task.DefinitionType)).Select(t => t.Task).ToList();
+                    if (nestedTaskGroup.All(t => existingMappings.Any(m => t.Id == m.SourceId)))
+                    {
+                        taskGroupsToMigrate.Add(taskGroup);
+                    }
+                }
+
+                nestedTaskGroups = nestedTaskGroups.Where(g => !taskGroupsToMigrate.Any(t => t.Id == g.Id)).ToList();
+                existingMappings = await CreateTaskGroupsAsync(serviceConnectionMappings, targetDefinitions, availableTasks, taskGroupsToMigrate, existingMappings);
+            } while (nestedTaskGroups.Any() && taskGroupsToMigrate.Any());


Infinite loop potential exists when nested task groups have circular dependencies or missing dependencies. The loop continues while nestedTaskGroups.Any() is true, but if taskGroupsToMigrate is empty (no progress made), it should exit to prevent indefinite execution. Consider adding a check to break when taskGroupsToMigrate is empty before awaiting CreateTaskGroupsAsync, or add iteration limit with appropriate logging.

Copilot · 2025-10-17T19:08:48Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

+                        var newInputs = new Dictionary<string, object>();
+                        foreach (var input in (IDictionary<String, Object>)task.Inputs)
+                        {
+                            var mapping = serviceConnectionMappings.FirstOrDefault(d => d.SourceId == input.Value.ToString());


Potential NullReferenceException if input.Value is null. The code calls .ToString() on input.Value without null checking. Add a null check before calling ToString() or use null-conditional operator: input.Value?.ToString()

Suggested change

var mapping = serviceConnectionMappings.FirstOrDefault(d => d.SourceId == input.Value.ToString());

var mapping = serviceConnectionMappings.FirstOrDefault(d => d.SourceId == input.Value?.ToString());

Copilot · 2025-10-17T19:08:48Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

+                            ((IDictionary<String, Object>)task.Inputs).Remove(input.Key);
+                            ((IDictionary<String, Object>)task.Inputs).Add(input.Key, input.Value);


This code removes and re-adds dictionary entries unnecessarily. Instead of removing and adding, directly update the value: ((IDictionary<String, Object>)task.Inputs)[input.Key] = input.Value;

Suggested change

((IDictionary<String, Object>)task.Inputs).Remove(input.Key);

((IDictionary<String, Object>)task.Inputs).Add(input.Key, input.Value);

((IDictionary<String, Object>)task.Inputs)[input.Key] = input.Value;

Copilot · 2025-10-17T19:08:48Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

            targetDefinitions = await Target.GetApiDefinitionsAsync<TaskGroup>(queryForDetails: false);
-            mappings.AddRange(FindExistingMappings(sourceDefinitions, targetDefinitions.Where(d => d.Name != null), mappings));
+            mappings.AddRange(FindExistingMappings(rootSourceDefinitions, targetDefinitions.Where(d => d.Name != null), mappings));
+            mappings.AddRange(existingMappings);


Duplicate mappings may be added to the list. The existingMappings are being added after potentially creating new mappings from the same source definitions in line 732. This could result in duplicate entries if the same mapping exists in both collections. Consider using mappings = mappings.Union(existingMappings).ToList() or check for duplicates before adding.

Suggested change

mappings.AddRange(existingMappings);

mappings = mappings.Union(existingMappings).ToList();

tomfrenzel

The new suggested changes by Copilot are valid. Please apply them and respond to by existing comments 😉

tomfrenzel · 2025-10-08T13:09:05Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs


            targetDefinitions = await Target.GetApiDefinitionsAsync<TaskGroup>(queryForDetails: false);
            mappings.AddRange(FindExistingMappings(sourceDefinitions, targetDefinitions.Where(d => d.Name != null), mappings));
+            mappings.AddRange(existingMappings);


Why do we need this? As far as i see, there are no changes to this list. Therefore we can just reuse it in the method calling this method

tomfrenzel · 2025-10-08T13:09:57Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

+                }
+
+                nestedTaskGroups = nestedTaskGroups.Where(g => !taskGroupsToMigrate.Any(t => t.Id == g.Id)).ToList();
+                existingMappings = await CreateTaskGroupsAsync(serviceConnectionMappings, targetDefinitions, availableTasks, taskGroupsToMigrate, existingMappings);


I think we need to apped the new mappings here instead of overriding the whole list

tomfrenzel · 2025-10-08T13:11:10Z

src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDevOpsPipelineProcessor.cs

            var filteredTaskGroups = FilterOutExistingTaskGroups(sourceDefinitions, targetDefinitions);
            filteredTaskGroups = FilterOutIncompatibleTaskGroups(filteredTaskGroups, availableTasks).ToList();

+            var existingMappings = FindExistingMappings(sourceDefinitions, targetDefinitions, new List<Mapping>());


Maybe instead of inititalizing a new list here, amke the parameter optional and initialize it in the method itself if not set

add support for nested task groups

6c2b758

Dev-Op requested a review from MrHinsh as a code owner September 30, 2025 16:38

MrHinsh requested review from Copilot and tomfrenzel October 1, 2025 15:04

MrHinsh approved these changes Oct 1, 2025

View reviewed changes

Copilot AI reviewed Oct 1, 2025

View reviewed changes

tomfrenzel suggested changes Oct 1, 2025

View reviewed changes

MrHinsh marked this pull request as draft October 1, 2025 21:02

MrHinsh and others added 7 commits October 7, 2025 20:22

Merge branch 'main' into main

82b12be

Merge branch 'main' into main

4f9bcb6

Update src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDe…

05170b5

…vOpsPipelineProcessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDe…

3f7b3a8

…vOpsPipelineProcessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDe…

7384a69

…vOpsPipelineProcessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

Update src/MigrationTools.Clients.AzureDevops.Rest/Processors/AzureDe…

828df80

…vOpsPipelineProcessor.cs Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

fix issues from automatic PR

591a12c

add IsMetaTask method for multiple used checks

MrHinsh requested review from Copilot and tomfrenzel October 17, 2025 19:07

Copilot AI reviewed Oct 17, 2025

View reviewed changes

tomfrenzel suggested changes Oct 25, 2025

View reviewed changes

	var mapping = serviceConnectionMappings.FirstOrDefault(d => d.SourceId == input.Value.ToString());
	var mapping = serviceConnectionMappings.FirstOrDefault(d => d.SourceId == input.Value?.ToString());

		((IDictionary<String, Object>)task.Inputs).Remove(input.Key);
		((IDictionary<String, Object>)task.Inputs).Add(input.Key, input.Value);

	mappings.AddRange(existingMappings);
	mappings = mappings.Union(existingMappings).ToList();

Uh oh!

Uh oh!

add support for nested task groups #3007

Are you sure you want to change the base?

add support for nested task groups #3007

Uh oh!

Conversation

Dev-Op commented Sep 30, 2025

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!

tomfrenzel left a comment

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!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Oct 17, 2025

Choose a reason for hiding this comment

Uh oh!

tomfrenzel left a comment

Choose a reason for hiding this comment

Uh oh!

tomfrenzel Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

tomfrenzel Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

tomfrenzel Oct 8, 2025

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants