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

Test PR to test copilot reviews #4212

Closed
wants to merge 3 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
196 changes: 139 additions & 57 deletions src/Microsoft.DotNet.Darc/Darc/Operations/GetChannelsOperation.cs
Original file line number Diff line number Diff line change
Expand Up @@ -2,94 +2,176 @@
// The .NET Foundation licenses this file to you under the MIT license.

using System;
using System.Collections.Generic;
using System.Linq;
using System.Threading.Tasks;
using Microsoft.DotNet.Darc.Options;
using Microsoft.DotNet.DarcLib;
using Microsoft.DotNet.Maestro.Client;
using Microsoft.DotNet.Maestro.Client.Models;
using Microsoft.DotNet.DarcLib.Helpers;
using Microsoft.DotNet.DarcLib.Models.VirtualMonoRepo;
using Microsoft.DotNet.DarcLib.VirtualMonoRepo;
using Microsoft.Extensions.Logging;
using Newtonsoft.Json;

namespace Microsoft.DotNet.Darc.Operations;

internal class GetChannelsOperation : Operation
class CodeFlowConflictResolver
{
private readonly GetChannelsCommandLineOptions _options;
private readonly IBarApiClient _barClient;
private readonly ILogger<GetChannelOperation> _logger;
private readonly IVmrInfo _vmrInfo;
private readonly ILocalGitRepoFactory _localGitRepoFactory;
private readonly ISourceManifest _sourceManifest;
private readonly IFileSystem _fileSystem;
private readonly ILogger<CodeFlowConflictResolver> _logger;

public GetChannelsOperation(
GetChannelsCommandLineOptions options,
IBarApiClient barClient,
ILogger<GetChannelOperation> logger)
public CodeFlowConflictResolver(
IVmrInfo vmrInfo,
ILocalGitRepoFactory localGitRepoFactory,
ISourceManifest sourceManifest,
IFileSystem fileSystem,
ILogger<CodeFlowConflictResolver> logger)
{
_options = options;
_barClient = barClient;
_vmrInfo = vmrInfo;
_localGitRepoFactory = localGitRepoFactory;
_sourceManifest = sourceManifest;
_fileSystem = fileSystem;
_logger = logger;
}

/// <summary>
/// Retrieve information about channels
/// </summary>
/// <param name="options">Command line options</param>
/// <returns>Process exit code.</returns>
public override async Task<int> ExecuteAsync()
public async Task<bool> TryMergingTargetBranch(string mappingName, string baseBranch, string targetBranch)
{
try
var vmr = _localGitRepoFactory.Create(_vmrInfo.VmrPath);
await vmr.CheckoutAsync(baseBranch);
var result = await vmr.RunGitCommandAsync(["merge", "--no-commit", "--no-ff", targetBranch]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { "merge", "--no-commit", "--no-ff", targetBranch }' instead.

Suggested change
var result = await vmr.RunGitCommandAsync(["merge", "--no-commit", "--no-ff", targetBranch]);
var result = await vmr.RunGitCommandAsync(new string[] { "merge", "--no-commit", "--no-ff", targetBranch });

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
if (result.Succeeded)
{
_logger.LogInformation("Successfully merged the branch {targetBranch} into {headBranch} in {repoPath}",
targetBranch,
baseBranch,
_vmrInfo.VmrPath);
await vmr.CommitAsync($"Merging {targetBranch} into {baseBranch}", allowEmpty: true);
return true;
}

result = await vmr.RunGitCommandAsync(["diff", "--name-only", "--diff-filter=U", "--relative"]);
if (!result.Succeeded)
{
var allChannels = await _barClient.GetChannelsAsync();
switch (_options.OutputFormat)
_logger.LogInformation("Failed to merge the branch {targetBranch} into {headBranch} in {repoPath}",
targetBranch,
baseBranch,
_vmrInfo.VmrPath);
result = await vmr.RunGitCommandAsync(["merge", "--abort"]);
return false;
}

var conflictedFiles = result.StandardOutput
.Split(Environment.NewLine, StringSplitOptions.RemoveEmptyEntries)
.Select(line => new UnixPath(line.Trim()));

var gitInfoFile = VmrInfo.GitInfoSourcesDir + "/" + mappingName + ".props";

foreach (var file in conflictedFiles)
{
// Known conflict in source-manifest.json
if (file == VmrInfo.DefaultRelativeSourceManifestPath)
{
case DarcOutputType.json:
WriteJsonChannelList(allChannels);
break;
case DarcOutputType.text:
WriteYamlChannelList(allChannels);
break;
default:
throw new NotImplementedException($"Output format {_options.OutputFormat} not supported for get-channels");
await TryResolvingSourceManifestConflict(vmr, mappingName);
continue;
}

return Constants.SuccessCode;
// Known conflict in a git-info props file - we just use our version as we expect it to be newer
// TODO: For batched subscriptions, we need to handle all git-info files
if (file == gitInfoFile)
{
await vmr.RunGitCommandAsync(["checkout", "--ours", file]);
await vmr.StageAsync([file]);
continue;
}

_logger.LogInformation("Failed to resolve conflicts in {file} between branches {targetBranch} and {headBranch} in {repoPath}",
file,
targetBranch,
baseBranch,
_vmrInfo.VmrPath);
result = await vmr.RunGitCommandAsync(["merge", "--abort"]);
return false;
}
catch (AuthenticationException e)

_logger.LogInformation("Successfully resolved version file conflicts between branches {targetBranch} and {headBranch} in {repoPath}",
targetBranch,
baseBranch,
_vmrInfo.VmrPath);
await vmr.CommitAsync($"Resolving conflicts between {targetBranch} and {baseBranch}", allowEmpty: false);
return true;
}

// TODO: This won't work for batched subscriptions
private async Task TryResolvingSourceManifestConflict(ILocalGitRepo vmr, string mappingName)
{
// We load the source manifest from the target branch and replace the current mapping (and its submodules) with our branches' information
var result = await vmr.RunGitCommandAsync(["show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { "show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath }' instead.

Suggested change
var result = await vmr.RunGitCommandAsync(["show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath]);
var result = await vmr.RunGitCommandAsync(new string[] { "show", "MERGE_HEAD:" + VmrInfo.DefaultRelativeSourceManifestPath });

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

var theirSourceManifest = SourceManifest.FromJson(result.StandardOutput);
var ourSourceManifest = _sourceManifest;
var updatedMapping = ourSourceManifest.Repositories.First(r => r.Path == mappingName);

theirSourceManifest.UpdateVersion(mappingName, updatedMapping.RemoteUri, updatedMapping.CommitSha, updatedMapping.PackageVersion, updatedMapping.BarId);

foreach (var submodule in theirSourceManifest.Submodules.Where(s => s.Path.StartsWith(mappingName + "/")))
{
Console.WriteLine(e.Message);
return Constants.ErrorCode;
theirSourceManifest.RemoveSubmodule(submodule);
}
catch (Exception e)

foreach (var submodule in _sourceManifest.Submodules.Where(s => s.Path.StartsWith(mappingName + "/")))
{
_logger.LogError(e, "Error: Failed to retrieve channels");
return Constants.ErrorCode;
theirSourceManifest.UpdateSubmodule(submodule);
}

_fileSystem.WriteToFile(_vmrInfo.SourceManifestPath, theirSourceManifest.ToJson());
_sourceManifest.Refresh(_vmrInfo.SourceManifestPath);
await vmr.StageAsync([_vmrInfo.SourceManifestPath]);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { _vmrInfo.SourceManifestPath }' instead.

Suggested change
await vmr.StageAsync([_vmrInfo.SourceManifestPath]);
await vmr.StageAsync(new string[] { _vmrInfo.SourceManifestPath });

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
}
}

private static void WriteJsonChannelList(IEnumerable<Channel> allChannels)
internal class GetChannelsOperation : Operation
{
private readonly GetChannelsCommandLineOptions _options;
private readonly IVmrCloneManager _cloneManager;
private readonly IVmrInfo _vmrInfo;
private readonly CodeFlowConflictResolver _conflictResolver;

public GetChannelsOperation(
GetChannelsCommandLineOptions options,
IVmrCloneManager cloneManager,
IVmrInfo vmrInfo,
CodeFlowConflictResolver conflictResolver)
{
var channelJson = new
{
channels = allChannels.OrderBy(c => c.Name).Select(channel =>
new
{
id = channel.Id,
name = channel.Name
})
};

Console.WriteLine(JsonConvert.SerializeObject(channelJson, Formatting.Indented));
_options = options;
_cloneManager = cloneManager;
_vmrInfo = vmrInfo;
_conflictResolver = conflictResolver;
}

private static void WriteYamlChannelList(IEnumerable<Channel> allChannels)
/// <summary>
/// Retrieve information about channels
/// </summary>
/// <param name="options">Command line options</param>
/// <returns>Process exit code.</returns>
public override async Task<int> ExecuteAsync()
{
// Write out a simple list of each channel's name
foreach (var channel in allChannels.OrderBy(c => c.Name))
var path = @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Hardcoded paths should be avoided in production code. Consider using a configuration setting or environment variable instead.

Suggested change
var path = @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";
var path = Environment.GetEnvironmentVariable("VmrPath") ?? @"C:\Users\prvysoky\AppData\Local\Temp\_vmrTests\wvzmghdz.ec2\_tests\xeihkfov.lgk\vmr";

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
var targetBranch = "main";
var prBranch = "OutOfOrderMergesTest-ff";

_vmrInfo.VmrPath = new NativePath(path);
await _cloneManager.PrepareVmrAsync([path], [targetBranch, prBranch], prBranch, default);
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

Array literals should be created using the 'new' keyword. Use 'new string[] { path }' and 'new string[] { targetBranch, prBranch }' instead.

Suggested change
await _cloneManager.PrepareVmrAsync([path], [targetBranch, prBranch], prBranch, default);
await _cloneManager.PrepareVmrAsync(new string[] { path }, new string[] { targetBranch, prBranch }, prBranch, default);

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

if (await _conflictResolver.TryMergingTargetBranch("product-repo1", prBranch, targetBranch))
{
// Pad so that id's up to 9999 will result in consistent
// listing
string idPrefix = $"({channel.Id})".PadRight(7);
Console.WriteLine($"{idPrefix}{channel.Name}");
Console.WriteLine("yay");
}
else
{
Console.WriteLine("nay");
}

return Constants.SuccessCode;
}
}
3 changes: 3 additions & 0 deletions src/Microsoft.DotNet.Darc/Darc/Options/CommandLineOptions.cs
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
using Microsoft.DotNet.Darc.Operations;
using Microsoft.DotNet.DarcLib;
using Microsoft.DotNet.DarcLib.Helpers;
using Microsoft.DotNet.DarcLib.VirtualMonoRepo;
using Microsoft.Extensions.DependencyInjection;
using Microsoft.Extensions.DependencyInjection.Extensions;
using Microsoft.Extensions.Logging;
Expand Down Expand Up @@ -172,6 +173,8 @@ public virtual IServiceCollection RegisterServices(IServiceCollection services)
);
services.TryAddSingleton<IRemoteTokenProvider>(_ => new RemoteTokenProvider(AzureDevOpsPat, GitHubPat));
services.TryAddSingleton<ICommandLineOptions>(_ => this);
services.AddTransient<CodeFlowConflictResolver>();
Copy link
Preview

Copilot AI Dec 4, 2024

Choose a reason for hiding this comment

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

The lifetime of CodeFlowConflictResolver should be changed to scoped to match the lifetime of IVmrCloneManager, as CodeFlowConflictResolver depends on IVmrCloneManager.

Suggested change
services.AddTransient<CodeFlowConflictResolver>();
services.AddScoped<CodeFlowConflictResolver>();

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
services.AddScoped<IVmrCloneManager, VmrCloneManager>();

return services;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,11 +3,12 @@

using CommandLine;
using Microsoft.DotNet.Darc.Operations;
using Microsoft.DotNet.Darc.Options.VirtualMonoRepo;

namespace Microsoft.DotNet.Darc.Options;

[Verb("get-channels", HelpText = "Get a list of channels.")]
internal class GetChannelsCommandLineOptions : CommandLineOptions<GetChannelsOperation>
internal class GetChannelsCommandLineOptions : VmrCommandLineOptions<GetChannelsOperation>
{
public override bool IsOutputFormatSupported()
=> OutputFormat switch
Expand Down
5 changes: 5 additions & 0 deletions src/Microsoft.DotNet.Darc/DarcLib/ILocalGitRepo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -178,4 +178,9 @@ Task CommitAsync(
/// <param name="args">Where to add the new argument into</param>
/// <param name="envVars">Where to add the new variables into</param>
public void AddGitAuthHeader(IList<string> args, IDictionary<string, string> envVars, string repoUri);

/// <summary>
/// Runs an arbitrary git command in the repo.
/// </summary>
Task<ProcessExecutionResult> RunGitCommandAsync(string[] args, CancellationToken cancellationToken = default);
}
3 changes: 3 additions & 0 deletions src/Microsoft.DotNet.Darc/DarcLib/LocalGitRepo.cs
Original file line number Diff line number Diff line change
Expand Up @@ -86,6 +86,9 @@ public async Task SetConfigValue(string setting, string value)
public async Task ResetWorkingTree(UnixPath? relativePath = null)
=> await _localGitClient.ResetWorkingTree(new NativePath(Path), relativePath);

public async Task<ProcessExecutionResult> RunGitCommandAsync(string[] args, CancellationToken cancellationToken = default)
=> await _localGitClient.RunGitCommandAsync(Path, args, cancellationToken);

public async Task StageAsync(IEnumerable<string> pathsToStage, CancellationToken cancellationToken = default)
=> await _localGitClient.StageAsync(Path, pathsToStage, cancellationToken);

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@ public string GetPublicUrl()
public interface IVersionedSourceComponent : ISourceComponent
{
string? PackageVersion { get; }
public int? BarId { get; }
}

/// <summary>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,8 +19,8 @@ public interface ISourceManifest

string ToJson();
void RemoveRepository(string repository);
void RemoveSubmodule(SubmoduleRecord submodule);
void UpdateSubmodule(SubmoduleRecord submodule);
void RemoveSubmodule(ISourceComponent submodule);
void UpdateSubmodule(ISourceComponent submodule);
void UpdateVersion(string repository, string uri, string sha, string? packageVersion, int? barId);
VmrDependencyVersion? GetVersion(string repository);
bool TryGetRepoVersion(string mappingName, [NotNullWhen(true)] out ISourceComponent? mapping);
Expand Down Expand Up @@ -54,15 +54,8 @@ public void UpdateVersion(string repository, string uri, string sha, string? pac
{
repo.CommitSha = sha;
repo.RemoteUri = uri;

if (packageVersion != null)
{
repo.PackageVersion = packageVersion;
}
if (barId != null)
{
repo.BarId = barId;
}
repo.PackageVersion = packageVersion;
repo.BarId = barId;
}
else
{
Expand All @@ -81,7 +74,7 @@ public void RemoveRepository(string repository)
_submodules.RemoveWhere(s => s.Path.StartsWith(repository + "/"));
}

public void RemoveSubmodule(SubmoduleRecord submodule)
public void RemoveSubmodule(ISourceComponent submodule)
{
var repo = _submodules.FirstOrDefault(r => r.Path == submodule.Path);
if (repo != null)
Expand All @@ -90,7 +83,7 @@ public void RemoveSubmodule(SubmoduleRecord submodule)
}
}

public void UpdateSubmodule(SubmoduleRecord submodule)
public void UpdateSubmodule(ISourceComponent submodule)
{
var repo = _submodules.FirstOrDefault(r => r.Path == submodule.Path);
if (repo != null)
Expand All @@ -100,7 +93,7 @@ public void UpdateSubmodule(SubmoduleRecord submodule)
}
else
{
_submodules.Add(submodule);
_submodules.Add(new SubmoduleRecord(submodule.Path, submodule.RemoteUri, submodule.CommitSha));
}
}

Expand Down
Loading
Loading