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

Ability to distinguish between changed and affected projects #81

Open
AlexeyRaga opened this issue Jun 14, 2023 · 9 comments
Open

Ability to distinguish between changed and affected projects #81

AlexeyRaga opened this issue Jun 14, 2023 · 9 comments

Comments

@AlexeyRaga
Copy link

Currently the tool helpfully reports all the changed or affected projects, both in .proj and in json.
One thing that is missing is to be able to know what project have actually changed and what are affected by the change.

Reason

This information is extremely useful in certain CI/CD scenarious.
For example, when we want to automatically bump the major version for the project that were actually changed, but the affected projects would probably get only a patch version bump.

Possible implementation

I see that internally the tool does have all this information, and I think that it'd be beneficial to propagate it into the output.

Json output can contain a new State field:

[
  {
    "Name": "Project1",
    "FilePath": "/path/to/project1.csproj",
    "State": "Changed"
  },
  {
    "Name": "Project2",
    "FilePath": "/path/to/project2.csproj",
    "State": "Affected"
  },
]

And for MSBuild we can have two item groups (Changed and Affected):

<Project Sdk="Microsoft.Build.Traversal/3.0.3">
  <ItemGroup Label="Changed">
    <ProjectReference Include="/path/to/project1.csproj" />
  </ItemGroup>
  <ItemGroup Label="Affected">
    <ProjectReference Include="/path/to/project2.csproj" />
  </ItemGroup>
</Project>
@leonardochaia
Copy link
Owner

Hi @AlexeyRaga I agree with the proposal and I think being able to differentiate from Changed vs Affected projects on the output is useful 👍

Would you be willing to give it a go implementing it?

@AlexeyRaga
Copy link
Author

I may give it a try. Let's see what I can come up with.
My first thought would be to add ProjectStatus into IProjectInfo and start from there...

@leonardochaia
Copy link
Owner

Hmmm, what do you think if we modify the IOutputFormatter.Execute to receive both lists of changed and affected projects?

I think that would be easier to implement, since this is only a formatting concern instead of the affected core logic. The core logic already keep two separate list for affected and changed, but they are joined together just for the output formatting part:

var allProjects = summary
.ProjectsWithChangedFiles
.Concat(summary.AffectedProjects)
.Select(p => new ProjectInfo(p));
// Generate output using formatters
var outputOptions = ctx.GetAffectedCommandOutputOptions(options);
var formatterExecutor = new OutputFormatterExecutor(console);
await formatterExecutor.Execute(
allProjects,
outputOptions.Formatters,
outputOptions.OutputDir,
outputOptions.OutputName,
outputOptions.DryRun,
verbose);

So I think it would be best for the IOutputFormatterExecutor and its IOutputFormatters to receive both lists and they can decide how to output them.

In the case of the JSON formatter, it needs to compose the output you've mentioned in your issue, so we need to work a bit in the output data structure (new class with the Changed state also implementing IProjectInfo perhaps?)

The Traversal formatter having the two list separate would be easier to achieve the proposal

And we also need to define what/how to do the text formatter, but I haven't thought about it yet.

Let me know if you hit a road block and we'll see what we can do, feel free to ask 👍

@AlexeyRaga
Copy link
Author

Thanks for the suggestion, I will see into it!
I will probably need some help at some point because it looks like the project is a bit Windows-oriented and I have some troubles with running it on MacOS, but I'll have a look if I can work it around (or can rely on CI for QA)

@leonardochaia
Copy link
Owner

leonardochaia commented Jun 14, 2023

Hi @AlexeyRaga , it should work on MacOS, we actually have CI tests that cover that.

I personally develop on Linux. Let me know if I can be of any help :)

@leonardochaia
Copy link
Owner

Sorry @AlexeyRaga did not mean to close the issue. Fat fingers.

@leonardochaia leonardochaia reopened this Jun 15, 2023
@leonardochaia
Copy link
Owner

Hi @AlexeyRaga, I was thinking about this last night and played a bit with the code. I still don't have a functioning prototype but wanted to leave some notes here:

dotnet-affected changes to existing output formatters:

  1. traversal (single file, separated by ItemGroup metadata)
  2. json (single file, separated by project object property)
  3. text (single file, no separation) (no changes)

dotnet-affected new formatters:

  1. json-split (affected.json, changed.json, excluded.json)
  2. traversal-split (affected.proj, changed.proj, excluded.proj)
  3. text-split (affected.txt, changed.txt, excluded.txt)

Some notes

  1. Not sure if the split should generate the excluded.json or not.
  2. Should we prefix all files with a well-known prefix/suffix for easier deletion? (rm *-affected.*)

@AlexeyRaga
Copy link
Author

AlexeyRaga commented Jun 16, 2023

Hi, good ideas!

I am not sure whether excluded* files would be useful, but why not have them for completeness? I guess, it won't be hard to generate them.

I like the idea about prefix/suffix, too (except that I would use . as a separator and not -).

Common suffix is nice for deletion, because it can be just rm *.affected.json
Common prefix is nice for files to be displayed together in a files list in IDE or ls, etc.

Maybe both them? Something like...

split-changes.affected.json
split-affected.affected.json
split-excluded.affected.json

Or, maybe, when split format is in play, we treat output-name as a directory and generate 3 files (changes.json, affected.json, excluded.json) in such a folder?
It won't "pollute" the files list this way, and it would be easy to gitignore/remove...

@jensk-dev
Copy link

jensk-dev commented Oct 6, 2024

Hi there,

I've added a simple proof of concept in #98 (exlcuding unit tests) to implement the following functionality:

  • Results can either be output to split or combined files using the --output-strategy split or --output-strategy combined.
  • Whatever is output can be selected by using the --output-filter argument, and specifying one or multiple of "affected", "changed", "excluded".

I still need to test it properly, however, I would like approval of my concept before I continue.

Cheers

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 a pull request may close this issue.

3 participants