add exclude path feature #70

Shikugawa · 2021-04-02T11:03:53Z

This PR introduces to skip check while filePath walk.

laurentsimon · 2021-07-28T00:56:56Z

@mco-gh @TomerAberbach do you have cycles to review this PR?
We need support for this for Google's scorecard project. How can we be unblocked?

laurentsimon · 2021-07-28T01:01:08Z

@dlorenc whom shall we talk to have this PR reviewed/merged?

mco-gh · 2021-07-28T07:59:37Z

I can take a look later today.

…

On Wed, 28 Jul 2021 at 02:01, laurentsimon ***@***.***> wrote: @dlorenc <https://github.com/dlorenc> whom shall we talk to have this PR reviewed/merged? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFAXF3MKMB7II7QRPUFFW3TZ5JF5ANCNFSM42IWOGOA> .

-- Marc Cohen Web: mco.dev Email: ***@***.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! <https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit> Q: Why is this email three sentences or less? A: three.sentenc.es

mco-gh · 2021-07-28T12:30:22Z

main.go

@@ -52,6 +52,7 @@ var (
 	license   = flag.String("l", "apache", "license type: apache, bsd, mit, mpl")
 	licensef  = flag.String("f", "", "license file")
 	year      = flag.String("y", fmt.Sprint(time.Now().Year()), "copyright year(s)")
+	exclude   = flag.String("e", "", "Files which exclude from check")


"Files to exclude from check" seems more accurate

mco-gh · 2021-07-28T12:40:58Z

main.go

+
+func (e *ExcludePath) Contains(path string) bool {
+	for _, p := range e.paths {
+		if p == path {


Will this work if someone specifies a relative path, e.g. /foo/bar, ./foo, ../foo/bar?
Perhaps you should canonicalize both strings to a full path to ensure you catch all those cases.

mco-gh · 2021-07-28T12:49:06Z

main_test.go

+
+	cmd := exec.Command(os.Args[0],
+		"-test.run=TestExcludePath",
+		"-y", "2018", "-e", origin1, origin1, origin2,


Two questions: 1) this is excluding a file. Isn't the more likely use case to exclude an entire dir tree? If so, could you add a test for that case? 2) Is the two appearances of origin1 a mistake or because you want to verify the logic handles duplicate paths?

In this implementation, I expect that only to accept filename. for example in current implementation, we get /foo/bar, /foo/bar/baz.txt after filepath walk and configured /foo/bar as exclude path then /foo/bar/baz.txt won't be ignored.

It also includes dup path test.

I think it'd be beneficial to support directories: the main use case (and ours!) is dev teams removing test folders from the path. Test folders have a number of files that change over time, so it's cumbersome to update the command every time a new file is added.

I agree - I think dirs is likely to be a far more valuable use case than enumerating individual files. If you do a prefix match then you get both, dirs and files, with a one line change.

mco-gh · 2021-07-28T13:22:45Z

Left comments.

…

On Wed, 28 Jul 2021 at 08:59, Marc Cohen ***@***.***> wrote: I can take a look later today. On Wed, 28 Jul 2021 at 02:01, laurentsimon ***@***.***> wrote: > @dlorenc <https://github.com/dlorenc> whom shall we talk to have this PR > reviewed/merged? > > — > You are receiving this because you were mentioned. > Reply to this email directly, view it on GitHub > <#70 (comment)>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AAFAXF3MKMB7II7QRPUFFW3TZ5JF5ANCNFSM42IWOGOA> > . > -- Marc Cohen Web: mco.dev Email: ***@***.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! <https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit> Q: Why is this email three sentences or less? A: three.sentenc.es

-- Marc Cohen Web: mco.dev Email: ***@***.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! <https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit> Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris · 2021-07-28T23:54:39Z

gah, I did it again! I started working on a feature before realizing there is another PR in flight. Well in any event, it may be worth a look, since I ended up taking a bit more of a flexible option by using https://github.com/bmatcuk/doublestar. This allows us to deprecate the existing -skip which is pretty limited in only supporting file extensions.

You can see the implementation here, and the test cases give a pretty clear example of what kinds of patterns would be possible: willnorris@6ac0919. This implementation may still suffer from the relative vs absolute path issue discussed here. I haven't looked closely enough yet to see if you all settled on a solution to that yet.

mco-gh · 2021-07-29T06:43:49Z

I like this implementation. I'm less concerned about abs/rel paths than the fact that #82 doesn't handle dirs, only files. Your PR handles both so I'm inclined to use it. Give a little time to review before merging.

…

On Thu, 29 Jul 2021 at 00:54, Will Norris ***@***.***> wrote: gah, I did it again! I started working on a feature before realizing there is another PR in flight. Well in any event, it may be worth a look, since I ended up taking a bit more of a flexible option by using https://github.com/bmatcuk/doublestar. This allows us to deprecate the existing -skip which is pretty limited in only supporting file extensions. You can see the implementation here, and the test cases give a pretty clear example of what kinds of patterns would be possible: willnorris@ 6ac0919 <willnorris@6ac0919>. This implementation may still suffer from the relative vs absolute path issue discussed here. I haven't looked closely enough yet to see if you all settled on a solution to that yet. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFAXF5XGHNVBGBOVXUXMQ3T2CKEVANCNFSM42IWOGOA> .

-- Marc Cohen Web: mco.dev Email: ***@***.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! <https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit> Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris · 2021-07-29T15:02:41Z

It's also worth noting, and I only realized it later, that doublestar requires go1.16 because it's using the io/fs package. In #81, I have it testing against the last two minor releases, which would include 1.15. That's what we do for google/go-github, and what the Go project itself supports. Go typically releases at the end of July, so go1.17 should drop any day now. In that case, we might wait until go1.17 releases before adding in doublestar, if that's the approach you'd like to go with. Then we can continue to support the last two minor releases.

mco-gh · 2021-07-29T15:36:07Z

Works for me.

…

On Thu, 29 Jul 2021 at 16:02, Will Norris ***@***.***> wrote: It's also worth noting, and I only realized it later, that doublestar requires go1.16 because it's using thew io/fs package. In #81 <#81>, I have it testing against the last two minor releases, which would include 1.15. That's what we do for google/go-github, and I *think* what the Go project itself supports (though I'm having trouble finding the docs). Go typically releases at the end of July, so go1.17 should drop any day now. In that case, we might wait until go1.17 releases before adding in doublestar, if that's the approach you'd like to go with. Then we can continue to support the last two minor releases. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#70 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAFAXFZXGNCIJ7STWADXWQTT2FUR3ANCNFSM42IWOGOA> .

-- Marc Cohen Web: mco.dev Email: ***@***.*** Working with me: mco.dev/working-with-marc Feedback: How am I doing? Provide anonymous feedback! <https://www.increment.me/feedback/049628fd-7339-406d-9203-17d3ec8976bd/submit> Q: Why is this email three sentences or less? A: three.sentenc.es

willnorris · 2021-08-09T19:54:40Z

alternate implementation of this feature was added in #84

add exclude path feature

7589893

Shikugawa mentioned this pull request Apr 2, 2021

Add support for gRPC calls. proxy-wasm/proxy-wasm-rust-sdk#100

Merged

naveensrinivasan mentioned this pull request Jul 28, 2021

BUG- Addlicense exclude path ossf/scorecard#767

Closed

mco-gh suggested changes Jul 28, 2021

View reviewed changes

willnorris mentioned this pull request Jul 29, 2021

support ignoring file or directories #84

Merged

willnorris closed this Aug 9, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add exclude path feature #70

add exclude path feature #70

Shikugawa commented Apr 2, 2021

laurentsimon commented Jul 28, 2021

laurentsimon commented Jul 28, 2021

mco-gh commented Jul 28, 2021 via email

mco-gh Jul 28, 2021

mco-gh Jul 28, 2021

mco-gh Jul 28, 2021

Shikugawa Jul 28, 2021 •

edited

Loading

laurentsimon Jul 28, 2021 •

edited

Loading

mco-gh Jul 28, 2021

mco-gh commented Jul 28, 2021 via email

willnorris commented Jul 28, 2021

mco-gh commented Jul 29, 2021 via email

willnorris commented Jul 29, 2021 •

edited

Loading

mco-gh commented Jul 29, 2021 via email

willnorris commented Aug 9, 2021

add exclude path feature #70

add exclude path feature #70

Conversation

Shikugawa commented Apr 2, 2021

laurentsimon commented Jul 28, 2021

laurentsimon commented Jul 28, 2021

mco-gh commented Jul 28, 2021 via email

mco-gh Jul 28, 2021

Choose a reason for hiding this comment

mco-gh Jul 28, 2021

Choose a reason for hiding this comment

mco-gh Jul 28, 2021

Choose a reason for hiding this comment

Shikugawa Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

laurentsimon Jul 28, 2021 • edited Loading

Choose a reason for hiding this comment

mco-gh Jul 28, 2021

Choose a reason for hiding this comment

mco-gh commented Jul 28, 2021 via email

willnorris commented Jul 28, 2021

mco-gh commented Jul 29, 2021 via email

willnorris commented Jul 29, 2021 • edited Loading

mco-gh commented Jul 29, 2021 via email

willnorris commented Aug 9, 2021

Shikugawa Jul 28, 2021 •

edited

Loading

laurentsimon Jul 28, 2021 •

edited

Loading

willnorris commented Jul 29, 2021 •

edited

Loading