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

[Rush] Add .changeignore file support #2279

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

Conversation

mmkal
Copy link
Contributor

@mmkal mmkal commented Oct 13, 2020

Fixes #2263

rush change will now look for a .changeignore file in each project with changed files. If it finds one, it will filter out files similarly to .gitignore, .npmignore, .eslintignore etc.

For example, to make sure test changes don't result in unwanted publishes, you could add **/*.test.ts to each project's .changeignore file.

mmkal added 4 commits October 13, 2020 15:24
`rush change` will now look for a `.changeignore` file in each project with changed files. If it finds one, it will filter out files similarly to `.gitignore`, `.npmignore`, `.eslintignore` etc.

For example, to make sure test changes don't result in unwanted publishes, you could add `**/*.test.ts` to each projet's `.changeignore` file.
Copy link
Contributor Author

@mmkal mmkal left a comment

Choose a reason for hiding this comment

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

CC @iclanton - again I couldn't find where tests for this kind of thing live. I don't want to invent testing conventions so would be great to get some help/guidance re how you think this should be tested. It's pretty straightforward and the interesting parts are delegated to the ignore package, but still would be good to make sure it doesn't have some boolean error somewhere.

apps/rush-lib/src/cli/actions/ChangeAction.ts Show resolved Hide resolved
apps/rush-lib/src/cli/actions/ChangeAction.ts Show resolved Hide resolved
apps/rush-lib/src/cli/actions/ChangeAction.ts Outdated Show resolved Hide resolved
@iclanton iclanton changed the title Add .changeignore file support [Rush] Add .changeignore file support Oct 14, 2020
@mmkal mmkal requested a review from iclanton October 19, 2020 19:29

const changeIgnoreFilePath: string = path.join(fsPath, RushConstants.changeignoreFileName);

return FileSystem.exists(changeIgnoreFilePath)
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return FileSystem.exists(changeIgnoreFilePath)
try {
return [...parentIgnoreStrings, FileSystem.readFile(changeIgnoreFilePath);
} catch (error) {
if (FileSystem.isNotExistsError(error)) {
return parentIgnoreStrings;
}
throw error;
}

Uses half as many file system operations, since FileSystem.exists() is internally just a try/catch around an fs.stat call.

Copy link
Contributor

@dmichon-msft dmichon-msft left a comment

Choose a reason for hiding this comment

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

I have performance concerns with the change as currently implemented. Path.isUnderOrEqual and Path.isUnder are currently hideously expensive, and this change causes the same (total changed files) * (number of projects) performance relationship that PackageChangeAnalyzer was experiencing until I refactored it to use the new findProjectByPosixRelativePath function to linearize the relationship.

It might be fine in a small repo, but it'll have very visible scaling influences for large PRs (e.g. bulk file moves) in a larger monorepo.

? [...parentIgnoreStrings, FileSystem.readFile(changeIgnoreFilePath)]
: parentIgnoreStrings;
});

this.rushConfiguration.projects
Copy link
Contributor

Choose a reason for hiding this comment

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

I recommend inverting the lookup:

const changedFilesByProject: Map<RushConfigurationProject, string[]> = new Map();

for (const file of changedFiles) {
  const relevantProject: RushConfigurationProject | undefined = this.rushConfiguration.findProjectByPosixRelativePath(file);
  if (!relevantProject) {
    continue;
  }
  let projectFiles: string[] | undefined = changedFilesByProject.get(relevantProject);
  if (!projectFiles) {
    if (!project.shouldPublish || project.versionPolicy && project.versionPolicy.exemptFromRushChange) {
      continue;
    }
    projectFiles = [];
    changedFilesByProject.set(relevantProject, projectFiles);
  }

  // Potentially test the file here
  projectFiles.push(file);
}

for (const [project, relevantFiles] of changedFilesByProject) {
  // Do your ignorer config processing and filtering here, and find out what is left.
  // Might consider recording which files prompted the change for posterity, so that we can have a --verbose option or similar for rush change that logs which files required the change file.
}

return [];
}
const changedPackageNames: Set<string> = new Set<string>();

const repoRootFolder: string | undefined = VersionControl.getRepositoryRootPath();
const projectHostMap: Map<string, string> = this._generateHostMap();

const getChangeIgnorePatterns: (fsPath: string) => string[] = memoize((fsPath: string): string[] => {
if ((repoRootFolder && Path.isUnder(repoRootFolder, fsPath)) || !FileSystem.exists(fsPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Be advised that, as currently implemented, this function is hideously expensive.

return [];
}
const changedPackageNames: Set<string> = new Set<string>();

const repoRootFolder: string | undefined = VersionControl.getRepositoryRootPath();
const projectHostMap: Map<string, string> = this._generateHostMap();

const getChangeIgnorePatterns: (fsPath: string) => string[] = memoize((fsPath: string): string[] => {
if ((repoRootFolder && Path.isUnder(repoRootFolder, fsPath)) || !FileSystem.exists(fsPath)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This will skip checking the entire tree for a deleted folder, which means that if said folder was supposed to be ignored for change detection, it will no longer be. I recommend skipping the check.

if ((repoRootFolder && Path.isUnder(repoRootFolder, fsPath)) || !FileSystem.exists(fsPath)) {
return [];
}
const parentIgnoreStrings: string[] = getChangeIgnorePatterns(path.dirname(fsPath));
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't ignore patterns defined relative to the file that defines them?


const ignoreConfig: Ignore = ignore();

for (const ignorePattern of flatMap(relevantFiles.map(path.dirname), getChangeIgnorePatterns)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's almost certainly worth deduplicating intermediary values to improve on performance.

const foldersToConsider: Set<string> = new Set(relevantFiles.map(path.dirname));
for (const folder of foldersToConsider) {
  const parent: string = path.dirname(folder);
  if (/* is parent in project */) {
    foldersToConsider.add(parent);
  }

  // Can lookup the local ignore file and add the patterns here, including relevant prefixing.
}

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

Successfully merging this pull request may close these issues.

[rush] exclude files from rush change diff calculation
3 participants