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

Add globbing support #1524

Closed
wants to merge 2 commits into from
Closed

Add globbing support #1524

wants to merge 2 commits into from

Conversation

lishaduck
Copy link

@lishaduck lishaduck commented Jul 25, 2024

Fixes #770 for real (and fixes #1582).

Personally, I think this change belongs in the cli, but I decided that rewriting the argument parsing there was a much larger endeavor than this.
Tests are failing because it now globs the files, but they don't actually exist. I suppose we need fixtures then? Shouldn't be breaking in practice though.

@ssbarnea
Copy link

ssbarnea commented Aug 13, 2024

I can wait to see this merged. Interesting how the globbing was silently removed on each major version...

@lishaduck lishaduck force-pushed the globs branch 3 times, most recently from bfb40d9 to f0bf605 Compare August 13, 2024 17:42
@lishaduck
Copy link
Author

I tried deduping deps, but it only found duplicate devdeps, so the build didn't get any smaller 😢

@glen-84
Copy link

glen-84 commented Sep 11, 2024

It might make sense to use @actions/glob, like @actions/upload-artifact does?

@lishaduck
Copy link
Author

It might make sense to use @actions/glob, like @actions/upload-artifact does?

I hadn't heard of that, but yes, it looks like a better alternative.

@glen-84 glen-84 mentioned this pull request Sep 29, 2024
@lishaduck
Copy link
Author

@glen-84, I did a quick look at bundle size; for a PR that drastically increases the size of the the package, I feel it's worth trying to keep it down—@actions/glob is a 1.65MB minimatch wrapper, while tinyglobby is only 146KB.

@glen-84
Copy link

glen-84 commented Oct 26, 2024

@lishaduck

That seems to be a known issue: actions/toolkit#1560, actions/toolkit#1697.

I suppose the one benefit of using the "official" package is that it's likely to already be installed. However, I don't feel strongly about it, and we can wait to see what the maintainers suggest. 🙂

@lishaduck
Copy link
Author

I suppose the one benefit of using the "official" package is that it's likely to already be installed.

Actions are pre-bundled, it'd get reinstalled.

However, I don't feel strongly about it, and we can wait to see what the maintainers suggest. 🙂

They're currently doing a rewrite in bash, see #1591 (comment).

@glen-84
Copy link

glen-84 commented Oct 27, 2024

Actions are pre-bundled, it'd get reinstalled.

Oh, that's a strange thing to do. 😕

They're currently doing a rewrite in bash, see #1591 (comment).

I saw this, but I misread it as being related to the eslint setup. 🙂


We've actually just switched to using the default file search mode where you don't specify the files. Probably not the most efficient, but it works for now.

@lishaduck
Copy link
Author

Oh, that's a strange thing to do. 😕

Yeah, actions are weird.

We've actually just switched to using the default file search mode where you don't specify the files. Probably not the most efficient, but it works for now.

That's what I've been doing as well, works well enough for now.

@lishaduck
Copy link
Author

Closing due to the v5 refactor; +1 #1582 instead.

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.

Add globbing support Globs not supported in v2?
3 participants