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

Cannot represent matching all files against multiple patterns #423

Closed
luisrayas3 opened this issue Aug 12, 2022 · 18 comments
Closed

Cannot represent matching all files against multiple patterns #423

luisrayas3 opened this issue Aug 12, 2022 · 18 comments
Assignees
Labels
feature request New feature or request to improve the current logic

Comments

@luisrayas3
Copy link

The use-case seems like it would be quite useful and natural: I would like a label to apply if ALL of the changed files are of a few certain types. For example:

documentation:
  - all: ["**/*.md", "**/*.puml"]

This of course doesn't work as all files need to match both patterns which never happens. Another attempt:

documentation:
  - all: ["**/*.md"]
  - all: ["**/*.puml"]

This almost works but does not work if the change touches both md files and puml files.

AFAICT there is no way to represent what I'm trying to do, but it feels like a very natural and useful case to support.

@MaksimZhukov MaksimZhukov added the feature request New feature or request to improve the current logic label Dec 12, 2022
@ilya-lavrenov
Copy link

Why this does not work?

documentation:
  - "**/*.md"
  - "**/*.puml"

@dfandrich
Copy link
Contributor

My reading of the documentation is that that example is the same as:

documentation:
  - any: ["**/*.md"]
  - any: ["**/*.puml"]

This would add the documentation label to a PR consisting of 99 .c files and one .md file, which is not the intention.

dfandrich added a commit to dfandrich/labeler that referenced this issue Jan 20, 2023
This is true when all files match at any of the patterns.
dfandrich added a commit to dfandrich/labeler that referenced this issue Jan 20, 2023
This is true when all files match any of the patterns.
@dfandrich
Copy link
Contributor

Just so it's explicitly clear, I have PR #487 outstanding that adds this feature.

@MaksimZhukov MaksimZhukov self-assigned this Jul 11, 2023
@MaksimZhukov MaksimZhukov linked a pull request Jul 12, 2023 that will close this issue
@silverwind
Copy link
Contributor

See comment #487 (comment) which illustrates my idea of how this filter would work.

@MaksimZhukov
Copy link
Contributor

Hello everyone!
We've released a new beta version of the action with this feature on board. We would appreciate it if you could use the new version and share with us your feedback.

@dfandrich
Copy link
Contributor

That looks like it satisfies this feature nicely! What's not clear from the documentation is what happens when multiple of AnyGlobToAllFiles, AnyGlobToAnyFile appear in a changed-files section. To all of them need to match for the label to be added, or any one of them? Or does the higher-level any: or all: answer that? It's not clear.

@MaksimZhukov
Copy link
Contributor

Hello @dfandrich!
You are right, top-level keys (any and all) bind options (changed-files, base-branch, etc.), glob combinations (AnyGlobToAnyFile, AllGlobsToAnyFile, etc.) and regexps. I've updated the release notes.
Thank you!

@MaksimZhukov
Copy link
Contributor

Hello everyone!
We have released the new major version of the action that includes the implementation of this feature. Please read the action documentation to find out how to adapt your configuration files and workflows for use with the new action version.

Thank you all for your patience!
Do not hesitate to contact us if you have any questions!

@jnewb1
Copy link

jnewb1 commented Dec 6, 2023

how would you accomplish the situation laid out in this issue with the new syntax? our goal is to avoid false positives when a bunch of files are changed in the PR that aren't actually relevant to the applied label

heres a table that I think describes where each of the existing rules fails in this use case

pattern changed files result does not solve issue, because
any-glob-to-all-files: ['**/*.md', **/*.puml] [test.md, test.puml] no match all files dont match any of the globs
any-glob-to-any-files: ['**/*.md', **/*.puml] [test.md, test.puml, test.test] match test.test doesn't match any regex, but label is still applied
all-glob-to-any-files: ['**/*.md', **/*.puml] [test.md] no match no .puml files are changed
all-glob-to-all-files: ['**/*.md', **/*.puml] [test.md] no match no .puml files are changed

@dfandrich
Copy link
Contributor

@jnew1 this is what I would expect for those four patterns you show:

  1. match (both files find a pattern that matches)
  2. match (test.md matches *.md, and that's all you need)
  3. no match (there's no file matching *.puml)
  4. no match (test.md doesn't match *.puml)

You wouldn't ever see a configuration like the last two, since they are impossible to ever be true. A more realistic case might be something like all-glob-to-all-files: ['README*', '*.txt'] where it's possible for a single file to match both patterns.

@jnewb1
Copy link

jnewb1 commented Dec 7, 2023

unfortunently, the first case doesn't match

I think this test describes exactly the problem within this issue

describe('check issue #423', () => {
  const changedFiles = ['test.md', 'test.puml'];

  describe('when at least one pattern matches all files', () => {
    const globPatterns = ['**/*.md', '**/*.puml'];

    it('returns true', () => {
      const result = checkIfAnyGlobMatchesAllFiles(
        changedFiles,
        globPatterns,
        false
      );
      expect(result).toBe(true);
    });
  });
});

for this to work, any-glob-to-all-files would need to be something more like

  • run glob1 on changedFiles
  • remove any files from changedFiles that match glob1
  • run glob2 on the remaining changedFiles
  • ... repeat ...
  • return true if changedFiles is empty

@dfandrich
Copy link
Contributor

The globPatterns in that test match only paths containing at least one slash. foo.md won't match **/*.md but bar/foo.md will. To also match files in the root of the repo, you need to specify const globPatterns = ['*.md', '**/*.md', '*.puml', '**/*.puml'];

@jnewb1
Copy link

jnewb1 commented Dec 8, 2023

same issue with this test

describe('check issue #423', () => {
  const changedFiles = ['test.md', 'test.puml'];

  describe('when at least one pattern matches all files', () => {
    const globPatterns = ['*.md', '*.puml'];

    it('returns true', () => {
      const result = checkIfAnyGlobMatchesAllFiles(
        changedFiles,
        globPatterns,
        false
      );
      expect(result).toBe(true);
    });
  });
});

@dfandrich
Copy link
Contributor

dfandrich commented Dec 8, 2023 via email

@jnewb1
Copy link

jnewb1 commented Dec 8, 2023

ANY one glob must match

yes that is exactly what it is

doesn't sounds very useful (or intuitive).

right

I believe I laid out a solution here: #423 (comment)

I can look at making a PR, but this would be an API breaking change, is it work making a new rule for it?

@jnewb1
Copy link

jnewb1 commented Dec 8, 2023

this new setup would break this test, which could be a legitimate use case:
https://github.com/actions/labeler/blob/8558fd74291d67161a8a78ce36a881fa63b766a9/__tests__/changedFiles.test.ts#L228C3-L239C6

so perhaps we just need a new rule

@dfandrich
Copy link
Contributor

Looks like you're right. I'd be up for renaming AnyGlobMatchesAllFiles to OneGlobMatchesAllFiles since that's what it does. Unless these semantics were used by mistake. I suspect the latter, because the current misnamed OneGlobMatchesAllFiles semantics could be implemented without it, in terms of AllGlobsMatchAllFiles, like:

- any
  - changed-files
    - all-globs-to-all-files: ['glob1']
    - all-globs-to-all-files: ['glob2']
    - all-globs-to-all-files: ['glob3']

whereas the reverse is not true. There is no way to implement AnyGlobMatchesAllFiles (the real one) in terms of the others.

@dfandrich
Copy link
Contributor

FWIW, #731 was opened about the issue we've been discussing in the latter comments.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature request New feature or request to improve the current logic
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants