Skip to content

Changes to Versions.props are always meaningful#5228

Merged
dkurepa merged 5 commits intodotnet:mainfrom
dkurepa:dkurepa/VersionsPropsChangesAreMeaningful
Aug 29, 2025
Merged

Changes to Versions.props are always meaningful#5228
dkurepa merged 5 commits intodotnet:mainfrom
dkurepa:dkurepa/VersionsPropsChangesAreMeaningful

Conversation

@dkurepa
Copy link
Copy Markdown
Member

@dkurepa dkurepa commented Aug 29, 2025

Copilot AI review requested due to automatic review settings August 29, 2025 09:29
Copy link
Copy Markdown
Contributor

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 PR fixes an issue where changes to Versions.props files in VMR (Virtual Mono Repo) repositories were being incorrectly filtered out as non-meaningful changes in the codeflow analysis. The fix ensures that all changes to Versions.props are treated as meaningful since VMR repos don't use automated dependency version management in these files.

Key Changes:

  • Modified the forward flow change analysis logic to exclude Versions.props from the list of dependency files that are filtered out
  • Added explanatory comment about VMR-specific behavior for Versions.props files

adamzip
adamzip previously approved these changes Aug 29, 2025
}.ToImmutableHashSet();

// In VMR repos, Versions.props doesn't contain any dependency versions maintained by automation, so every change is meaningful
public static ImmutableHashSet<string> CodeflowDependencyFiles { get; } = DependencyFiles.Except([VersionFiles.VersionsProps]).ToImmutableHashSet();
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: it might not make a difference, but i would err on the side of explicitly defining the files in CodeflowDpenendencyFiles rather than letting it depend on DependencyFiles

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What do you think about renaming DependencyFiles to something that shows it's strictly for non-codeflow subscriptions? Like NonCodeflowDependencyFiles?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

nit: it might not make a difference, but i would err on the side of explicitly defining the files in CodeflowDpenendencyFiles rather than letting it depend on DependencyFiles

I kind of like the fact that it's easy to see what the difference is. Also if we ever add a new dependency file for whatever reason, I think it'd likely go to both places, this was kind of a special case with us splitting a file, and forcing codeflow repos to move away from Versions.props being considered a dependency file

I'm fine with the renaming tho

premun
premun previously approved these changes Aug 29, 2025
}.ToImmutableHashSet();

// In VMR repos, Versions.props doesn't contain any dependency versions maintained by automation, so every change is meaningful
public static ImmutableHashSet<string> CodeflowDependencyFiles { get; } = NonCodeflowDependencyFiles.Except([VersionFiles.VersionsProps]).ToImmutableHashSet();
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

nit: could also be this:

Suggested change
public static ImmutableHashSet<string> CodeflowDependencyFiles { get; } = NonCodeflowDependencyFiles.Except([VersionFiles.VersionsProps]).ToImmutableHashSet();
public static ImmutableHashSet<string> CodeflowDependencyFiles { get; } = [..NonCodeflowDependencyFiles.Except([VersionFiles.VersionsProps])];

@dkurepa dkurepa merged commit b1fa1d0 into dotnet:main Aug 29, 2025
9 checks passed
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.

4 participants