Skip to content
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

Add flat flow test functionality to the ReproTool #4559

Open
wants to merge 7 commits into
base: main
Choose a base branch
from

Conversation

dkurepa
Copy link
Member

@dkurepa dkurepa commented Mar 13, 2025

@dkurepa
Copy link
Member Author

dkurepa commented Mar 13, 2025

we can check this code in, or just keep it on my branch, since we probably won't need it after switching to flat flow in a month

@premun premun requested a review from Copilot March 13, 2025 15:24
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request adds multiple new test operations to support flat and full flow tests in the ReproTool and refactors the options and service registration. Key changes include the introduction of FullBackflowTestOperation, FlatFlowTestOperation, and ReproOperation along with updates to the Program entry-point and shared library helper methods.

Reviewed Changes

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

Show a summary per file
File Description
tools/ProductConstructionService.ReproTool/Options/FullBackflowTestOptions.cs New options class for full backflow tests with duplicate license header comments.
tools/ProductConstructionService.ReproTool/Operations/FullBackflowTestOperation.cs New operation implementing the full backflow test functionality.
tools/ProductConstructionService.ReproTool/Options/FlatFlowTestOptions.cs New options class for flat flow tests.
tools/ProductConstructionService.ReproTool/Operations/ReproOperation.cs Updated operation for reproducing code flow subscriptions.
tools/ProductConstructionService.ReproTool/Operations/FlatFlowTestOperation.cs New operation for flat flow test execution.
tools/ProductConstructionService.ReproTool/Operations/Operation.cs Base class for common helper methods and constants shared across operations.
tools/ProductConstructionService.ReproTool/Program.cs Updated program to parse multiple verbs and use GetOperation to run the selected test.
tools/ProductConstructionService.ReproTool/Options/ReproOptions.cs Updated options for the repro operation with minor header duplication.
tools/ProductConstructionService.ReproTool/DarcProcessManager.cs Adjustments in subscription creation and command execution parameters.
tools/Tools.Common/Constants.cs, tools/FlatFlowMigrationCli/* Namespace and minor reference updates to support common functionality.
tools/ProductConstructionService.ReproTool/ReproTool.cs Removal of the legacy ReproTool class in favor of the new operation-based design.

@@ -1,18 +1,21 @@
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

// Licensed to the .NET Foundation under one or more agreements.
Copy link
Preview

Copilot AI Mar 13, 2025

Choose a reason for hiding this comment

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

Duplicate license header found. Remove the redundant header lines to streamline the file header.

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Comment on lines +4 to +6
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Licensed to the .NET Foundation under one or more agreements.
// The .NET Foundation licenses this file to you under the MIT license.

Comment on lines +30 to +31
[Option("github-token", HelpText = "GitHub token", Required = false)]
public string? GitHubToken { get; set; }
Copy link
Member

Choose a reason for hiding this comment

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

I'd kill this option and only allow it people to use user secrets or ENV variables

Comment on lines +19 to +20
[Option("target-branch", HelpText = "Target branch in all repos", Required = true)]
public required string TargetBranch { get; init; }
Copy link
Member

Choose a reason for hiding this comment

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

How does this work? Doesn't the branch differ per repo?
I think the branch you get from the dependency resolver should be the target branch.

var vmrRepos = await vmrDependencyResolver.GetVmrRepositoriesAsync(
"https://github.com/dotnet/dotnet",
"https://github.com/dotnet/sdk",
"main");
Copy link
Member

Choose a reason for hiding this comment

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

This should probably come from the Options.VmrBranch?

"https://github.com/dotnet/sdk",
"main");

vmrRepos = vmrRepos.Where(d => d.Mapping.Name == "runtime").ToList();
Copy link
Member

Choose a reason for hiding this comment

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

This probably stayed behind

Comment on lines +41 to +45
var productRepoForkUri = $"{ProductRepoFormat}{vmrRepo.Mapping.Name}";
if (vmrRepo.Mapping.Name == "nuget-client")
{
productRepoForkUri = $"{ProductRepoFormat}nuget.client";
}
Copy link
Member

Choose a reason for hiding this comment

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

The URI is in vmrRepo.Channel.Repository

protected const string VmrForkRepoName = "dotnet";
protected const string VmrForkUri = $"https://github.com/{MaestroAuthTestOrgName}/{VmrForkRepoName}";
protected const string ProductRepoFormat = $"https://github.com/{MaestroAuthTestOrgName}/";
protected const long InstallationId = 289474;
Copy link
Member

Choose a reason for hiding this comment

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

Is this really needed? The first subscription you add should look it up correctly.

Comment on lines +61 to +65
var productRepoForkUri = $"{ProductRepoFormat}{vmrRepo.Mapping.Name}";
if (vmrRepo.Mapping.Name == "nuget-client")
{
productRepoForkUri = $"{ProductRepoFormat}nuget.client";
}
Copy link
Member

Choose a reason for hiding this comment

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

Same as before

using ProductConstructionService.ReproTool.Operations;

namespace ProductConstructionService.ReproTool.Options;
[Verb("flat-flow-test", HelpText = "Test full flat flow in the maestro-auth-test org")]
Copy link
Member

Choose a reason for hiding this comment

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

Should this be forward flow test?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants