fix source/target directory + add source-commit option#5334
fix source/target directory + add source-commit option#5334adamzip merged 8 commits intodotnet:mainfrom
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR enhances the Product Construction Service ReproTool's FlowCommitOperation by adding support for specifying source commits directly and fixing source/target directory assignment logic. The changes improve flexibility in testing dependency flow scenarios.
Key changes:
- Added optional
source-commitparameter as an alternative tosource-branch - Refactored source/target directory assignment logic with proper forward/backward flow detection
- Simplified codeflow metadata detection using dedicated helper methods
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tools/ProductConstructionService.ReproTool/Options/FlowCommitOptions.cs | Added source-commit option and made source-branch optional |
| tools/ProductConstructionService.ReproTool/Operations/FlowCommitOperation.cs | Refactored directory assignment logic and added VMR detection methods |
…ptions.cs Co-authored-by: Copilot <[email protected]>
…itOperation.cs Co-authored-by: Copilot <[email protected]>
| _logger.LogInformation("Creating build for {repo}@{branch} (commit {commit})", | ||
| _options.SourceRepository, | ||
| _options.SourceBranch, |
There was a problem hiding this comment.
The log message references _options.SourceBranch which can now be null when source-commit is provided. This will log 'null' for the branch parameter when using the commit option.
| _logger.LogInformation("Creating build for {repo}@{branch} (commit {commit})", | |
| _options.SourceRepository, | |
| _options.SourceBranch, | |
| var branchOrCommit = !string.IsNullOrEmpty(_options.SourceBranch) | |
| ? _options.SourceBranch | |
| : Microsoft.DotNet.DarcLib.Commit.GetShortSha(sourceCommit); | |
| _logger.LogInformation("Creating build for {repo}@{branch} (commit {commit})", | |
| _options.SourceRepository, | |
| branchOrCommit, |
…ptions.cs Co-authored-by: Copilot <[email protected]>
| [Option("source-branch", HelpText = "Either --source-branch or --source-commit is required. Branch whose commit will be flown. Ignored if source-commit is provided.", Required = false)] | ||
| public string? SourceBranch { get; init; } | ||
|
|
||
| [Option("source-commit", HelpText = "Either --source-branch or --source-commit is required. Source commit that will be flown.", Required = false)] |
There was a problem hiding this comment.
How does this work with --realBuildId? Probably you can either supply a build or a commit? Or does the source commit override the build SHA? Or the other way?
There was a problem hiding this comment.
I haven't been using --realBuildId. Should I make it impossible to provide both a --sourceCommit and a --realBuildId?
There was a problem hiding this comment.
I think it might be easier to forbid providing both - build will just use it's own SHA.
If you also can rename it to --build-id it would better match what we have elsewhere.
There was a problem hiding this comment.
I think we use the realBuildId just for the assets. We don't use it for the sha, as in you can give a branch/sha that's not corresponding to the build
There was a problem hiding this comment.
So maybe it should be named like that - --assets-from-build or something?
#5335