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

Assigns labels based on branch names #203

Merged
merged 119 commits into from
May 24, 2023
Merged
Show file tree
Hide file tree
Changes from 21 commits
Commits
Show all changes
119 commits
Select commit Hold shift + click to select a range
2bf42e4
Add branch to MatchConfig interface
joshdales Aug 7, 2021
ad73546
Add function for checking branches
joshdales Aug 7, 2021
6c50d09
include checkBranch in checkMatch
joshdales Aug 7, 2021
ee0e0eb
Add a new fixture and test for the branch checking
joshdales Aug 7, 2021
a01b9ae
Add another test to make sure that partial branch naming works
joshdales Aug 7, 2021
bce88a9
Update new test description
joshdales Aug 7, 2021
765934f
Run build
joshdales Aug 7, 2021
827e118
Fix PR branch labeler
amiel Sep 15, 2021
8aa7614
Run build
amiel Sep 15, 2021
cb5f448
Use correct branch name and update tests
amiel Sep 15, 2021
2ced1f3
Run build
amiel Sep 15, 2021
d9ed3e8
Format
amiel Sep 15, 2021
79c0cc7
Include a test for branching
amiel Sep 15, 2021
27a1d89
Update src/labeler.ts
amiel Sep 16, 2021
7624214
Merge pull request #1 from bentohq/fix-branch-labeler
joshdales Sep 16, 2021
2d63815
Allow branch config to be an array as well
joshdales Sep 29, 2021
ab49f7a
Add tests for array branh labelling
joshdales Sep 29, 2021
2246b66
Run build
joshdales Sep 29, 2021
89f6b77
minor adjustment for successful branch matching.
joshdales Oct 29, 2021
7aadc17
Update the tests for applying multiple branch based labels
joshdales Oct 29, 2021
71fc664
run the build script
joshdales Oct 29, 2021
818399d
Merge branch 'main' into main
joshdales Jun 11, 2022
6e27606
Update README with reference to branch options
joshdales Jun 11, 2022
0ad789c
Merge branch 'main' into main
joshdales Aug 30, 2022
0861fa5
Run the build command
joshdales Aug 30, 2022
4c74e84
Fix typo in the README
joshdales Sep 22, 2022
7f8d8e4
Merge branch 'main' into main
joshdales Jan 11, 2023
c54c5a2
Run prettier
joshdales Jan 11, 2023
7b1327b
Run the build command
joshdales Jan 11, 2023
8c59ecc
Rename the getBranchName helper
joshdales Jan 28, 2023
f2b2513
Update the matching to use a regexp rather than minimatch
joshdales Jan 28, 2023
2343710
Move all the branch checking into its own file
joshdales Jan 28, 2023
cd3a8df
Create a test file for branch
joshdales Jan 28, 2023
0b6e68d
Add options for getting the head or base branch
joshdales Jan 28, 2023
2daf35a
Add an extra test now that we can check the base branch
joshdales Jan 28, 2023
231de6b
Remove the branch option and replace with just head-branch and base-b…
joshdales Jan 29, 2023
922ffdf
Remove deprecated IMinimatch import from labeler.ts
joshdales Jan 29, 2023
7a5c525
Create new interfaces for MatchConfig
joshdales Jan 29, 2023
969899d
Add the changedFiles key and update logic in labeler
joshdales Jan 29, 2023
7d17531
Update the labeler tests
joshdales Jan 29, 2023
b071d82
Run the build command
joshdales Jan 29, 2023
0eb9d49
reverse the conditions of checkMatch
joshdales Jan 29, 2023
09f0853
Fix some typos in the branch checks
joshdales Feb 19, 2023
ed31b27
Rename some functions and variables to match what they are doing
joshdales Feb 19, 2023
da83a18
Make sure that the changed files config values are an array
joshdales Feb 19, 2023
56347d5
Add unit tests for toBranchMatchConfig
joshdales Feb 21, 2023
8943ca2
Merge pull request #3 from joshdales/new-config-structure
joshdales Feb 21, 2023
e5b1bdd
Update the README with documentation about the new config structure
joshdales Feb 21, 2023
2e10ffb
Reference minimatch in docs
joshdales Mar 3, 2023
2a5bc55
Fix bad test descriptions
joshdales Mar 3, 2023
394a01b
Make getBranchName argument non-optional
joshdales Mar 3, 2023
5e6bdf6
update the example workflow in the readme
joshdales Mar 3, 2023
84e83a9
Merge branch 'main' into main
joshdales Mar 8, 2023
f40b387
Correct errors and typos in the README
joshdales Mar 17, 2023
90ef370
Merge branch 'actions:main' into main
joshdales Mar 18, 2023
3eec5d8
Update the syntax for checking a match
joshdales Mar 15, 2023
fc5eb71
Revert "reverse the conditions of checkMatch"
joshdales Mar 15, 2023
e4486e9
Throw an error if the config is the wrong type
joshdales Mar 18, 2023
e939550
Simplfy the conditions in toBranchMatchConfig
joshdales Mar 18, 2023
51b763c
Update comments in getMatchConfigs to represent updated types
joshdales Mar 18, 2023
c08f5fa
Condense assignment of further config options in toChangedFilesMatchC…
joshdales Mar 18, 2023
9f259ee
Rename checkGlobs to checkMatchConfigs and check each property
joshdales Mar 18, 2023
1ce9b35
Move all changedFiles logic into it's own file
joshdales Mar 18, 2023
ef108a9
Convert the yaml output to a matchConfig in getLabelConfigMapFromObject
joshdales Mar 18, 2023
a988f4e
Add todo tests for changedFiles
joshdales Mar 18, 2023
3af9a47
Add unit tests for toChangedFilesMatchConfig
joshdales Mar 18, 2023
c31ee1f
Add more unit tests for changedFiles.ts
joshdales Mar 18, 2023
17694aa
Update the argument type for toMatchConfig
joshdales Mar 18, 2023
e9a1777
Fix linting and formatting
joshdales Mar 18, 2023
65b7640
Add unit tests for toMatchConfig
joshdales Mar 18, 2023
7741e57
Run the build command
joshdales Mar 18, 2023
64ce5e9
Update README with better description for branches
joshdales Mar 20, 2023
cc1e025
Update object assignment in toChangedFilesMatchConfig
joshdales Mar 20, 2023
d0d3628
Add extra tests and use toEqual matcher.
joshdales Mar 20, 2023
92990c0
Don't allow empty changed-files objects through
joshdales Mar 20, 2023
d31255f
Make sure that empty config options don't accidently label things
joshdales Mar 20, 2023
b25e3a8
Better wording for the new test
joshdales Mar 20, 2023
1c9c27e
Run the build command
joshdales Mar 21, 2023
8e6367d
Merge branch 'main' into main
joshdales Mar 23, 2023
9bfc999
Run build and fix bad merge
joshdales Mar 23, 2023
5d0a66e
Run the build command again
joshdales Mar 23, 2023
e51b118
Change the structure of the config
joshdales Mar 25, 2023
a9e07ce
Add some new tests
joshdales Mar 25, 2023
3bec922
Add any and all functions for both checks
joshdales Mar 25, 2023
432b275
Get all the tests passings
joshdales Mar 25, 2023
4967646
Run the build command
joshdales Mar 25, 2023
4554c0d
Add a bunch more tests
joshdales Mar 25, 2023
5ac9519
Update the README
joshdales Mar 25, 2023
ef6ab1b
Add function for checking if any path matches
joshdales Mar 25, 2023
62f22bd
Run the build command
joshdales Mar 25, 2023
0b2cfb0
Im an idiot, bad copy pasta
joshdales Mar 25, 2023
29382eb
Build command
joshdales Mar 25, 2023
fa7f98c
Yikes, still missed that
joshdales Mar 25, 2023
210043e
Run the build command
joshdales Mar 25, 2023
938f9c9
Update the readme a little more
joshdales Mar 26, 2023
3ddce51
Update the debug values
joshdales Mar 27, 2023
4be192c
Run the build command
joshdales Mar 27, 2023
67604ee
Update more debugging statements
joshdales Mar 27, 2023
b1a2f85
Update debugging indentation on the branch checks
joshdales Mar 27, 2023
2f1dfd1
Adjust the indenting again
joshdales Mar 27, 2023
7f169bc
Merge pull request #4 from joshdales/another-config-setup
joshdales Mar 27, 2023
d4d4a10
Have a single isMatch for checking changed files
joshdales Apr 13, 2023
2637d23
Add test for when not all globs match any changed file
joshdales Apr 13, 2023
c1b0ca7
Run the build command
joshdales Apr 13, 2023
2a3422a
Better description for the new test
joshdales Apr 13, 2023
13e75b4
minor update to the readme
joshdales Apr 13, 2023
68a2598
Merge branch 'actions:main' into main
joshdales Apr 13, 2023
11812c3
Revert "Have a single isMatch for checking changed files"
joshdales May 5, 2023
9488def
Update tests and build
joshdales May 5, 2023
3aa0d43
Better test description
joshdales May 5, 2023
9cfddd0
Consolidate the new any change files test into the old one
joshdales May 5, 2023
51cc5e0
Update text in test descriptions and logging
joshdales May 11, 2023
09645fd
Move the allowed Matchconfig keys into a constant
joshdales May 11, 2023
34a5bf6
Update the validation when there are no options in the matchConfigs
joshdales May 11, 2023
4ac1764
Add a guard clause to stop false changed-files positives
joshdales May 11, 2023
a5bed11
Run the build command
joshdales May 11, 2023
a256a58
Add check for empty objects in checkAll
joshdales May 15, 2023
57d3407
Better check for empty configs in checkAll
joshdales May 17, 2023
3352df1
Bring test I accidently deleted
joshdales May 17, 2023
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions __tests__/fixtures/branches.yml
Original file line number Diff line number Diff line change
@@ -0,0 +1,11 @@
test-branch:
- branch: "test/**"

feature-branch:
- branch: "*/feature/*"

bug-branch:
- branch: "{bug,fix}/*"

array-branch:
- branch: ["array/*"]
59 changes: 59 additions & 0 deletions __tests__/main.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ const paginateMock = jest.spyOn(gh, "paginate");
const getPullMock = jest.spyOn(gh.rest.pulls, "get");

const yamlFixtures = {
"branches.yml": fs.readFileSync("__tests__/fixtures/branches.yml"),
"only_pdfs.yml": fs.readFileSync("__tests__/fixtures/only_pdfs.yml"),
};

Expand Down Expand Up @@ -102,6 +103,64 @@ describe("run", () => {
expect(addLabelsMock).toHaveBeenCalledTimes(0);
expect(removeLabelMock).toHaveBeenCalledTimes(0);
});

it("adds labels based on the branch names that match the glob pattern", async () => {
github.context.payload.pull_request!.head = { ref: "test/testing-time" };
usingLabelerConfigYaml("branches.yml");
await run();

expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["test-branch"],
});
});

it("adds multiple labels based on branch names that match different glob patterns", async () => {
github.context.payload.pull_request!.head = {
ref: "test/feature/123",
};
usingLabelerConfigYaml("branches.yml");
await run();

expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["test-branch", "feature-branch"],
});
});

it("it can support multiple branches by batching", async () => {
github.context.payload.pull_request!.head = { ref: "fix/123" };
usingLabelerConfigYaml("branches.yml");
await run();

expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["bug-branch"],
});
});

it("it can support multiple branches by providing an array", async () => {
github.context.payload.pull_request!.head = { ref: "array/123" };
usingLabelerConfigYaml("branches.yml");
await run();

expect(addLabelsMock).toHaveBeenCalledTimes(1);
expect(addLabelsMock).toHaveBeenCalledWith({
owner: "monalisa",
repo: "helloworld",
issue_number: 123,
labels: ["array-branch"],
});
});
});

function usingLabelerConfigYaml(fixtureName: keyof typeof yamlFixtures): void {
Expand Down
75 changes: 61 additions & 14 deletions dist/index.js
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,14 @@ function getPrNumber() {
}
return pullRequest.number;
}
function getBranchName() {
joshdales marked this conversation as resolved.
Show resolved Hide resolved
var _a;
const pullRequest = github.context.payload.pull_request;
if (!pullRequest) {
return undefined;
}
return (_a = pullRequest.head) === null || _a === void 0 ? void 0 : _a.ref;
}
function getChangedFiles(client, prNumber) {
return __awaiter(this, void 0, void 0, function* () {
const listFilesOptions = client.rest.pulls.listFiles.endpoint.merge({
Expand Down Expand Up @@ -203,6 +211,38 @@ function checkAll(changedFiles, globs) {
core.debug(` "all" patterns matched all files`);
return true;
}
function matchBranchPattern(matcher, branchName) {
core.debug(` - ${printPattern(matcher)}`);
if (matcher.match(branchName)) {
core.debug(` "branch" pattern matched`);
return true;
}
core.debug(` ${printPattern(matcher)} did not match`);
return false;
}
function checkBranch(glob) {
const branchName = getBranchName();
if (!branchName) {
core.debug(` no branch name`);
return false;
}
core.debug(` checking "branch" pattern against ${branchName}`);
if (Array.isArray(glob)) {
const matchers = glob.map((g) => new minimatch_1.Minimatch(g));
for (const matcher of matchers) {
if (matchBranchPattern(matcher, branchName)) {
core.debug(` "branch" patterns matched against ${branchName}`);
return true;
}
}
core.debug(` "branch" patterns did not match against ${branchName}`);
return false;
}
else {
const matcher = new minimatch_1.Minimatch(glob);
return matchBranchPattern(matcher, branchName);
}
}
function checkMatch(changedFiles, matchConfig) {
if (matchConfig.all !== undefined) {
if (!checkAll(changedFiles, matchConfig.all)) {
Expand All @@ -214,6 +254,11 @@ function checkMatch(changedFiles, matchConfig) {
return false;
}
}
if (matchConfig.branch !== undefined) {
if (!checkBranch(matchConfig.branch)) {
return false;
}
}
return true;
}
function addLabels(client, prNumber, labels) {
Expand Down Expand Up @@ -11709,103 +11754,103 @@ module.exports = eval("require")("encoding");
/***/ ((module) => {

"use strict";
module.exports = require("assert");;
module.exports = require("assert");

/***/ }),

/***/ 8614:
/***/ ((module) => {

"use strict";
module.exports = require("events");;
module.exports = require("events");

/***/ }),

/***/ 5747:
/***/ ((module) => {

"use strict";
module.exports = require("fs");;
module.exports = require("fs");

/***/ }),

/***/ 8605:
/***/ ((module) => {

"use strict";
module.exports = require("http");;
module.exports = require("http");

/***/ }),

/***/ 7211:
/***/ ((module) => {

"use strict";
module.exports = require("https");;
module.exports = require("https");

/***/ }),

/***/ 1631:
/***/ ((module) => {

"use strict";
module.exports = require("net");;
module.exports = require("net");

/***/ }),

/***/ 2087:
/***/ ((module) => {

"use strict";
module.exports = require("os");;
module.exports = require("os");

/***/ }),

/***/ 5622:
/***/ ((module) => {

"use strict";
module.exports = require("path");;
module.exports = require("path");

/***/ }),

/***/ 2413:
/***/ ((module) => {

"use strict";
module.exports = require("stream");;
module.exports = require("stream");

/***/ }),

/***/ 4016:
/***/ ((module) => {

"use strict";
module.exports = require("tls");;
module.exports = require("tls");

/***/ }),

/***/ 8835:
/***/ ((module) => {

"use strict";
module.exports = require("url");;
module.exports = require("url");

/***/ }),

/***/ 1669:
/***/ ((module) => {

"use strict";
module.exports = require("util");;
module.exports = require("util");

/***/ }),

/***/ 8761:
/***/ ((module) => {

"use strict";
module.exports = require("zlib");;
module.exports = require("zlib");

/***/ })

Expand Down Expand Up @@ -11844,7 +11889,9 @@ module.exports = require("zlib");;
/************************************************************************/
/******/ /* webpack/runtime/compat */
/******/
/******/ if (typeof __nccwpck_require__ !== 'undefined') __nccwpck_require__.ab = __dirname + "/";/************************************************************************/
/******/ if (typeof __nccwpck_require__ !== 'undefined') __nccwpck_require__.ab = __dirname + "/";
/******/
/************************************************************************/
var __webpack_exports__ = {};
// This entry need to be wrapped in an IIFE because it need to be in strict mode.
(() => {
Expand Down
1 change: 1 addition & 0 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

52 changes: 52 additions & 0 deletions src/labeler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import { Minimatch, IMinimatch } from "minimatch";
interface MatchConfig {
all?: string[];
any?: string[];
branch?: string | string[];
Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, the new field name in the yaml config file (branch) is not specific. We should make it more concrete so that there is no confusion and users can understand what type of branch is meant. How about headBranch?
There is a feature request in the repository to add similar functionality, but for base branches. It would be great if you could add this in scope of your PR. I think we can reuse some logic that was introduced by you.

Copy link
Contributor

Choose a reason for hiding this comment

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

At the moment, the new field (branch) is on the same level as the patterns for changed files. The current structure of the configuration file looks like this:

- label name:
  - patterns for changed files
  - branch:
    - patterns for branch

Potentially, more new fields will be added in the future and such a structure will become non-obvious, non-intuitive. I think we should introduce a new field for changed files as well:

- label name:
  - changed files:
    - patterns
  - branch:
    - patterns
... 

This will be a breaking change, but we're going to release a new major version anyway to fix the bug related to the sync-labels input. We can include both changes in the new major release.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Interesting so to follow up from your previous comment the we would actually wanna change this to the following:

- label name:
  - changed-files:
    - patterns
  - head-branch:
    - patterns 

So that we could add another level for base-branch in the future?

🤔 Or in the way that changed-files can have any and all we keep that level called branch as you suggest here and then add head and base options to that. So the config might end up looking like this:

- label name:
  - changed-files:
    - any: 
      - patterns
    - all:
      - patterns
  - branch:
    - head:
      - patterns
    - base:
      - patterns 

I like that idea 👍

Copy link
Contributor Author

@joshdales joshdales Jan 21, 2023

Choose a reason for hiding this comment

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

Small follow up question to that - what would we want the defaults to be in the case that these nested options are not provided?
eg.

- label 1: 'pattern'
- label 2:
  - changed-files: 'pattern'
- label 3:
  - branch: 'pattern'

Currently it defaults to match any file that has changed, would we still want to keep that as it is or adjust it? Based on how things are currently I guess it might be something like this:

  • label-1 matches any file that has changed (as it currently does)
  • label-2 also matches any file that has changed?
  • label-3 matches the head branch?

Copy link
Contributor

Choose a reason for hiding this comment

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

In my understanding, the final structure should look like this:

- label name:
  - changed-files:
    - patterns
    # or
    - any: 
      - patterns
    - all:
      - patterns
  - head-branch:
    - patterns
  - base-branch:
    - patterns 

We may want to expand branch settings, for example the base and/or head branches can have include or exclude options, so I expect them to be separate fields so that we can avoid deep nesting:

...
  - head-branch:
    - include:
      - patterns
    - exclude:
      - patterns
  - base-branch:
    - include:
      - patterns
    - exclude:
      - patterns
...

Regarding your question about omitting nested options - I think we should adhere the following approach:

  • label 1 - throw an error, because it is not obvious what these patterns are related to, what exactly the user meant.
  • label 2 - matches any file that has changed, as you mentioned
  • label 3 - Separating the branches, as I suggested, would simplify the logic:
  - head-branch:
    - patterns
  - base-branch:
    - patterns 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Would having include and exclude options more not come from the pattern that you pass? Regardless avoiding deeply nested objects makes sense for now so I'll start on adding that functionality 👍

}

type StringOrMatchConfig = string | MatchConfig;
Expand Down Expand Up @@ -71,6 +72,15 @@ function getPrNumber(): number | undefined {
return pullRequest.number;
}

function getBranchName(): string | undefined {
const pullRequest = github.context.payload.pull_request;
if (!pullRequest) {
return undefined;
}

return pullRequest.head?.ref;
}

async function getChangedFiles(
client: ClientType,
prNumber: number
Expand Down Expand Up @@ -213,6 +223,42 @@ function checkAll(changedFiles: string[], globs: string[]): boolean {
return true;
}

function matchBranchPattern(matcher: IMinimatch, branchName: string): boolean {
core.debug(` - ${printPattern(matcher)}`);
if (matcher.match(branchName)) {
core.debug(` "branch" pattern matched`);
return true;
}

core.debug(` ${printPattern(matcher)} did not match`);
return false;
}

function checkBranch(glob: string | string[]): boolean {
const branchName = getBranchName();
if (!branchName) {
core.debug(` no branch name`);
return false;
}

core.debug(` checking "branch" pattern against ${branchName}`);
if (Array.isArray(glob)) {
const matchers = glob.map((g) => new Minimatch(g));
for (const matcher of matchers) {
if (matchBranchPattern(matcher, branchName)) {
core.debug(` "branch" patterns matched against ${branchName}`);
return true;
}
}

core.debug(` "branch" patterns did not match against ${branchName}`);
return false;
} else {
const matcher = new Minimatch(glob);
return matchBranchPattern(matcher, branchName);
}
}

function checkMatch(changedFiles: string[], matchConfig: MatchConfig): boolean {
if (matchConfig.all !== undefined) {
if (!checkAll(changedFiles, matchConfig.all)) {
Expand All @@ -226,6 +272,12 @@ function checkMatch(changedFiles: string[], matchConfig: MatchConfig): boolean {
}
}

if (matchConfig.branch !== undefined) {
if (!checkBranch(matchConfig.branch)) {
return false;
}
}

return true;
}

Expand Down