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

Implement dot option #316

Merged
merged 29 commits into from
Jun 5, 2023
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
29 commits
Select commit Hold shift + click to select a range
b0d9292
Implement dot option
kachkaev Feb 4, 2022
6a0b726
Rebuild
kachkaev Feb 4, 2022
409a5a2
Fix typo in test caption
kachkaev Feb 4, 2022
920e9b6
Add comments
kachkaev Feb 4, 2022
8b8677b
Improve warning
kachkaev Feb 4, 2022
e05b7c1
Update README.md
kachkaev Feb 4, 2022
f3632d3
Update README.md
kachkaev Feb 4, 2022
d89797c
Merge remote-tracking branch 'upstream/main' into dot-option
kachkaev Sep 14, 2022
2025154
Simplify glob
kachkaev Oct 5, 2022
866eff5
Merge remote-tracking branch 'origin/main' into dot-option
kachkaev Jan 16, 2023
305cfeb
Rebuild
kachkaev Jan 16, 2023
4a96e77
Merge branch 'main' into dot-option
kachkaev Mar 12, 2023
b898cc8
Merge remote-tracking branch 'u/main' into dot-option
kachkaev Mar 27, 2023
b28379f
Cleanup
kachkaev Mar 27, 2023
a7cc8a6
Fix
kachkaev Mar 27, 2023
012b892
Merge remote-tracking branch 'u/main' into dot-option
kachkaev May 19, 2023
cecbd94
Merge remote-tracking branch 'u/main' into dot-option
kachkaev May 31, 2023
092c979
Update src/labeler.ts
kachkaev May 31, 2023
60f44e7
Fix unrelated test
kachkaev May 31, 2023
9776203
Update build
kachkaev May 31, 2023
a27020c
Undo unwanted change in diff
kachkaev May 31, 2023
44414db
Fix tests
kachkaev May 31, 2023
673c7e2
Rebuild
kachkaev May 31, 2023
e3b3815
Further diff reduction
kachkaev May 31, 2023
54d434d
Remove `required: false`
kachkaev May 31, 2023
59d3310
Rebuild
kachkaev May 31, 2023
71d2484
Address review comment
kachkaev May 31, 2023
639ba81
Rebuild
kachkaev May 31, 2023
d40596e
micromatch → minimatch
kachkaev Jun 2, 2023
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
32 changes: 26 additions & 6 deletions README.md
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ Automatically label new pull requests based on the paths of files being changed.

Create a `.github/labeler.yml` file with a list of labels and [minimatch](https://github.com/isaacs/minimatch) globs to match to apply the label.

The key is the name of the label in your repository that you want to add (eg: "merge conflict", "needs-updating") and the value is the path (glob) of the changed files (eg: `src/**/*`, `tests/*.spec.js`) or a match object.
The key is the name of the label in your repository that you want to add (eg: "merge conflict", "needs-updating") and the value is the path (glob) of the changed files (eg: `src/**`, `tests/*.spec.js`) or a match object.

#### Match Object

Expand Down Expand Up @@ -47,12 +47,17 @@ label1:

From a boolean logic perspective, top-level match objects are `OR`-ed together and individual match rules within an object are `AND`-ed. Combined with `!` negation, you can write complex matching rules.

> ⚠️ This action uses [micromatch](https://www.npmjs.com/package/micromatch) to apply glob patterns.
kachkaev marked this conversation as resolved.
Show resolved Hide resolved
> For historical reasons, paths starting with dot (e.g. `.github`) are not matched by default.
> You need to set `dot: true` to change this behavior.
> See [Inputs](#inputs) table below for details.

#### Basic Examples

```yml
# Add 'label1' to any changes within 'example' folder or any subfolders
label1:
- example/**/*
- example/**
Copy link
Contributor Author

Choose a reason for hiding this comment

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


# Add 'label2' to any file changes within 'example2' folder
label2: example2/*
Expand All @@ -67,16 +72,15 @@ repo:

# Add '@domain/core' label to any change within the 'core' package
@domain/core:
- package/core/*
kachkaev marked this conversation as resolved.
Show resolved Hide resolved
- package/core/**/*
- package/core/**

# Add 'test' label to any change to *.spec.js files within the source dir
test:
- src/**/*.spec.js

# Add 'source' label to any change to src files within the source dir EXCEPT for the docs sub-folder
source:
- any: ['src/**/*', '!src/docs/*']
- any: ['src/**', '!src/docs/*']

# Add 'frontend` label to any change to *.js files as long as the `main.js` hasn't changed
frontend:
Expand Down Expand Up @@ -116,7 +120,23 @@ Various inputs are defined in [`action.yml`](action.yml) to let you configure th
| `repo-token` | Token to use to authorize label changes. Typically the GITHUB_TOKEN secret, with `contents:read` and `pull-requests:write` access | N/A |
| `configuration-path` | The path to the label configuration file | `.github/labeler.yml` |
| `sync-labels` | Whether or not to remove labels when matching files are reverted or no longer changed by the PR | `false`
| `dot` | Whether or not to auto-include paths starting with dot (e.g. `.github`) | `false`

Choose a reason for hiding this comment

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

WDYT about dropping the configuration option and making dot always true.

If someone really wants to ignore dot files, that is another glob expression, right? (Something already supported.) IMO, if we can solve the same problem with less configuration it will be more user friendly and more likely to "do what I mean".

Copy link
Contributor Author

@kachkaev kachkaev Sep 15, 2022

Choose a reason for hiding this comment

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

That would be a breaking change which could upset existing users. It’s safer to introduce the option, then wait for a few months, change the default in a major release, wait again and finally remove the option completely.

Copy link

@jdufresne jdufresne Sep 15, 2022

Choose a reason for hiding this comment

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

Yes, I agree it changes the behavior, but perhaps it is a reasonable one. We could also bump the major version to play it safe with expectations. Ultimately, it is not my decision. It is only a suggestion as an end user.


When `dot` is disabled and you want to include _all_ files in a folder:

```yml
label1:
- path/to/folder/**/*
- path/to/folder/**/.*
Comment on lines +123 to +124
Copy link
Contributor Author

Choose a reason for hiding this comment

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

What about path/to/folder/sub/.folder/hello.txt. Will it match? 🤔 If not, what would be the right set of globs?

```

If `dot` is enabled:

```yml
label1:
- path/to/folder/**
```

# Contributions
## Contributions

Contributions are welcome! See the [Contributor's Guide](CONTRIBUTING.md).
18 changes: 16 additions & 2 deletions __tests__/labeler.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,15 +15,29 @@ const matchConfig = [{ any: ["*.txt"] }];
describe("checkGlobs", () => {
it("returns true when our pattern does match changed files", () => {
const changedFiles = ["foo.txt", "bar.txt"];
const result = checkGlobs(changedFiles, matchConfig);
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeTruthy();
});

it("returns false when our pattern does not match changed files", () => {
const changedFiles = ["foo.docx"];
const result = checkGlobs(changedFiles, matchConfig);
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeFalsy();
});

it("returns false for a file starting with dot if `dot` option is false", () => {
const changedFiles = [".foo.txt"];
const result = checkGlobs(changedFiles, matchConfig, false);

expect(result).toBeFalsy();
});

it("returns true for a file starting with dot if `dot` option is true", () => {
const changedFiles = [".foo.txt"];
const result = checkGlobs(changedFiles, matchConfig, true);

expect(result).toBeTruthy();
});
});
63 changes: 49 additions & 14 deletions __tests__/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -18,10 +18,24 @@ const yamlFixtures = {
"only_pdfs.yml": fs.readFileSync("__tests__/fixtures/only_pdfs.yml"),
};

const configureInput = (
mockInput: Partial<{
"repo-token": string;
"configuration-path": string;
"sync-labels": boolean;
dot: boolean;
}>
) => {
jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
};

afterAll(() => jest.restoreAllMocks());

describe("run", () => {
it("adds labels to PRs that match our glob patterns", async () => {
it("(with dot: false) adds labels to PRs that match our glob patterns", async () => {
configureInput({});
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.pdf");

Expand All @@ -37,7 +51,36 @@ describe("run", () => {
});
});

it("does not add labels to PRs that do not match our glob patterns", async () => {
it("(with dot: true) adds labels to PRs that match our glob patterns", async () => {
configureInput({ dot: true });
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles(".foo.pdf");

await run();

expect(removeLabelMock).toHaveBeenCalledTimes(0);
expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["touched-a-pdf-file"],
});
});

it("(with dot: false) does not add labels to PRs that do not match our glob patterns", async () => {
configureInput({});
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles(".foo.pdf");

await run();

expect(removeLabelMock).toHaveBeenCalledTimes(0);
expect(addLabelsMock).toHaveBeenCalledTimes(0);
});

it("(with dot: true) does not add labels to PRs that do not match our glob patterns", async () => {
configureInput({ dot: true });
usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");

Expand All @@ -48,15 +91,11 @@ describe("run", () => {
});

it("(with sync-labels: true) it deletes preexisting PR labels that no longer match the glob pattern", async () => {
let mockInput = {
configureInput({
"repo-token": "foo",
"configuration-path": "bar",
"sync-labels": true,
};

jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
});

usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");
Expand All @@ -79,15 +118,11 @@ describe("run", () => {
});

it("(with sync-labels: false) it issues no delete calls even when there are preexisting PR labels that no longer match the glob pattern", async () => {
let mockInput = {
configureInput({
"repo-token": "foo",
"configuration-path": "bar",
"sync-labels": false,
};

jest
.spyOn(core, "getInput")
.mockImplementation((name: string, ...opts) => mockInput[name]);
});

usingLabelerConfigYaml("only_pdfs.yml");
mockGitHubResponseChangedFiles("foo.txt");
Expand Down
4 changes: 4 additions & 0 deletions action.yml
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,10 @@ inputs:
description: 'Whether or not to remove labels when matching files are reverted'
default: false
required: false
dot:
description: 'Whether or not to auto-include paths starting with dot (e.g. `.github`)'
default: false
required: false

runs:
using: 'node16'
Expand Down
19 changes: 10 additions & 9 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ function run() {
const token = core.getInput("repo-token", { required: true });
const configPath = core.getInput("configuration-path", { required: true });
const syncLabels = !!core.getInput("sync-labels", { required: false });
const dot = !!core.getInput("dot", { required: false });
const prNumber = getPrNumber();
if (!prNumber) {
console.log("Could not get pull request number from context, exiting");
Expand All @@ -68,7 +69,7 @@ function run() {
const labelsToRemove = [];
for (const [label, globs] of labelGlobs.entries()) {
core.debug(`processing ${label}`);
if (checkGlobs(changedFiles, globs)) {
if (checkGlobs(changedFiles, globs, dot)) {
labels.push(label);
}
else if (pullRequest.labels.find((l) => l.name === label)) {
Expand Down Expand Up @@ -158,11 +159,11 @@ function toMatchConfig(config) {
function printPattern(matcher) {
return (matcher.negate ? "!" : "") + matcher.pattern;
}
function checkGlobs(changedFiles, globs) {
function checkGlobs(changedFiles, globs, dot) {
for (const glob of globs) {
core.debug(` checking pattern ${JSON.stringify(glob)}`);
const matchConfig = toMatchConfig(glob);
if (checkMatch(changedFiles, matchConfig)) {
if (checkMatch(changedFiles, matchConfig, dot)) {
return true;
}
}
Expand All @@ -182,8 +183,8 @@ function isMatch(changedFile, matchers) {
return true;
}
// equivalent to "Array.some()" but expanded for debugging and clarity
function checkAny(changedFiles, globs) {
const matchers = globs.map((g) => new minimatch_1.Minimatch(g));
function checkAny(changedFiles, globs, dot) {
const matchers = globs.map((g) => new minimatch_1.Minimatch(g, { dot }));
core.debug(` checking "any" patterns`);
for (const changedFile of changedFiles) {
if (isMatch(changedFile, matchers)) {
Expand All @@ -195,7 +196,7 @@ function checkAny(changedFiles, globs) {
return false;
}
// equivalent to "Array.every()" but expanded for debugging and clarity
function checkAll(changedFiles, globs) {
function checkAll(changedFiles, globs, dot) {
const matchers = globs.map((g) => new minimatch_1.Minimatch(g));
core.debug(` checking "all" patterns`);
for (const changedFile of changedFiles) {
Expand All @@ -207,14 +208,14 @@ function checkAll(changedFiles, globs) {
core.debug(` "all" patterns matched all files`);
return true;
}
function checkMatch(changedFiles, matchConfig) {
function checkMatch(changedFiles, matchConfig, dot) {
if (matchConfig.all !== undefined) {
if (!checkAll(changedFiles, matchConfig.all)) {
if (!checkAll(changedFiles, matchConfig.all, dot)) {
return false;
}
}
if (matchConfig.any !== undefined) {
if (!checkAny(changedFiles, matchConfig.any)) {
if (!checkAny(changedFiles, matchConfig.any, dot)) {
return false;
}
}
Expand Down
32 changes: 23 additions & 9 deletions src/labeler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ export async function run() {
const token = core.getInput("repo-token", { required: true });
const configPath = core.getInput("configuration-path", { required: true });
const syncLabels = !!core.getInput("sync-labels", { required: false });
const dot = !!core.getInput("dot", { required: false });

const prNumber = getPrNumber();
if (!prNumber) {
Expand All @@ -42,7 +43,7 @@ export async function run() {
const labelsToRemove: string[] = [];
for (const [label, globs] of labelGlobs.entries()) {
core.debug(`processing ${label}`);
if (checkGlobs(changedFiles, globs)) {
if (checkGlobs(changedFiles, globs, dot)) {
labels.push(label);
} else if (pullRequest.labels.find((l) => l.name === label)) {
labelsToRemove.push(label);
Expand Down Expand Up @@ -157,12 +158,13 @@ function printPattern(matcher: IMinimatch): string {

export function checkGlobs(
changedFiles: string[],
globs: StringOrMatchConfig[]
globs: StringOrMatchConfig[],
dot: boolean
): boolean {
for (const glob of globs) {
core.debug(` checking pattern ${JSON.stringify(glob)}`);
const matchConfig = toMatchConfig(glob);
if (checkMatch(changedFiles, matchConfig)) {
if (checkMatch(changedFiles, matchConfig, dot)) {
return true;
}
}
Expand All @@ -184,8 +186,12 @@ function isMatch(changedFile: string, matchers: IMinimatch[]): boolean {
}

// equivalent to "Array.some()" but expanded for debugging and clarity
function checkAny(changedFiles: string[], globs: string[]): boolean {
const matchers = globs.map((g) => new Minimatch(g));
function checkAny(
changedFiles: string[],
globs: string[],
dot: boolean
): boolean {
const matchers = globs.map((g) => new Minimatch(g, { dot }));
core.debug(` checking "any" patterns`);
for (const changedFile of changedFiles) {
if (isMatch(changedFile, matchers)) {
Expand All @@ -199,7 +205,11 @@ function checkAny(changedFiles: string[], globs: string[]): boolean {
}

// equivalent to "Array.every()" but expanded for debugging and clarity
function checkAll(changedFiles: string[], globs: string[]): boolean {
function checkAll(
changedFiles: string[],
globs: string[],
dot: boolean
): boolean {
const matchers = globs.map((g) => new Minimatch(g));
core.debug(` checking "all" patterns`);
for (const changedFile of changedFiles) {
Expand All @@ -213,15 +223,19 @@ function checkAll(changedFiles: string[], globs: string[]): boolean {
return true;
}

function checkMatch(changedFiles: string[], matchConfig: MatchConfig): boolean {
function checkMatch(
changedFiles: string[],
matchConfig: MatchConfig,
dot: boolean
): boolean {
if (matchConfig.all !== undefined) {
if (!checkAll(changedFiles, matchConfig.all)) {
if (!checkAll(changedFiles, matchConfig.all, dot)) {
return false;
}
}

if (matchConfig.any !== undefined) {
if (!checkAny(changedFiles, matchConfig.any)) {
if (!checkAny(changedFiles, matchConfig.any, dot)) {
return false;
}
}
Expand Down