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

[Do Not Review]fix common prefix for instrumentation filter #11690

Closed
wants to merge 1 commit into from

Conversation

caiyulun
Copy link

@caiyulun caiyulun commented Jul 1, 2020

fix #10949, we should remove nested filter only if it is a subpath of another filter.

@ulfjack
Copy link
Contributor

ulfjack commented Jul 1, 2020

Please add a test if possible, and also link to a bug - if there is one already, great, otherwise please file a bug.

@caiyulun caiyulun changed the title fix common prefix for instrumentation filter [Do not Review]fix common prefix for instrumentation filter Jul 1, 2020
@caiyulun
Copy link
Author

caiyulun commented Jul 1, 2020

Please add a test if possible, and also link to a bug - if there is one already, great, otherwise please file a bug.

Thanks, I'm working on unit test now.

@caiyulun caiyulun changed the title [Do not Review]fix common prefix for instrumentation filter [Do Not Review]fix common prefix for instrumentation filter Jul 1, 2020
@meisterT
Copy link
Member

What's the state of this PR?

@jin jin added the WIP label Oct 26, 2020
@jin jin added the team-Rules-Server Issues for serverside rules included with Bazel label Oct 26, 2020
@jin
Copy link
Member

jin commented Oct 26, 2020

Assigning back to the PR author to add tests.

@philwo
Copy link
Member

philwo commented Nov 10, 2020

We're replacing the "WIP" label with the native GitHub "draft PRs". I thus converted this PR to a draft.

Please mark this as ready for review when it's ready, or close it if you no longer intend to work on it. Thanks! 👍

@philwo philwo marked this pull request as draft November 10, 2020 17:28
@philwo philwo removed the WIP label Nov 10, 2020
@limdor
Copy link
Contributor

limdor commented Nov 28, 2020

Any status in this PR? Any objections if I add some tests to it and submit a PR? Maybe I will regret my words but should not be hard to add a test here.

@philwo philwo requested a review from c-mita December 2, 2020 15:46
@philwo
Copy link
Member

philwo commented Dec 2, 2020

@c-mita Is this something you could help with? (Just checked who touched this file last and it was you. ;))

@c-mita
Copy link
Member

c-mita commented Dec 2, 2020

This looks reasonable to me. Unfortunately the test cases for this class are internal (I don't know if there's anything blocking them from being open-sourced or if it's just a question of effort and time).

If you could add a single test case for the issue you're fixing that can be transplanted into the internal suite, that would be great. We did this for #11971.

@limdor
Copy link
Contributor

limdor commented Dec 2, 2020

@c-mita I tried to add them but I got blocked. I added a description on the original issue ticket. Maybe you know the answer? #10949

@aiuto
Copy link
Contributor

aiuto commented Oct 7, 2021

Friendly ping

@limdor
Copy link
Contributor

limdor commented Oct 7, 2021

I think this can be closed, see
#10949 and the pr associated to it limdor@2dad806

@aiuto aiuto closed this Oct 7, 2021
@aiuto
Copy link
Contributor

aiuto commented Oct 7, 2021 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes team-Rules-Server Issues for serverside rules included with Bazel
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default --instrumentation_filter when 2 targets share a prefix is incorrect
9 participants