Fix RegEx in FeatureFileOrchestrator to resolve file consent type #1645
Conversation
| @@ -64,7 +64,7 @@ internal class FeatureFileOrchestrator( | |||
| ) | |||
|
|
|||
| companion object { | |||
| private const val BASE_DIR_NAME_REG_EX = "([a-z]+[-|_])+" | |||
| private const val BASE_DIR_NAME_REG_EX = "([a-z]+(-|_))+" | |||
There was a problem hiding this comment.
Do we have a list of all the available names so far? Why are we including the _ character?
There was a problem hiding this comment.
the _ is used in the NDK folders for some reason...I see it was not aligned with the new folders name format. For now all the other features are using the - separator. That's the list so far:
logs-pending-v2
logs-v2
ndk_crash_reports_intermediary_v2
ndk_crash_reports_v2
rum-pending-v2
rum-v2
session-replay-pending-v2
session-replay-v2
tracing-pending-v2
tracing-v2
There was a problem hiding this comment.
NDK feature doesn't use storage for batches, so we may exclude folders belonging to it, it is outside of the scope for file orchestrator
There was a problem hiding this comment.
ok that's great...and the way I see it neither the uploader is using it as the ndk_crash_file is just used to write the equivalent RUM event in their (pending|granted) folder at runtime ? Can you confirm this @0xnm ...this way I could remove that extra rule from there
There was a problem hiding this comment.
so it seems you can simplify that regex to ([a-z]+-)+
There was a problem hiding this comment.
yes, NDK folders are out of scope of the upload pipeline
There was a problem hiding this comment.
@xgouchet yup doing this now after the confirmation ;)
mariusc83
force-pushed
the
mconstantin/fix-reg-ex-in-fetaure-file-orchestrator
branch
from
9ea793f to
8b5140f
Compare
| @@ -64,7 +64,7 @@ internal class FeatureFileOrchestrator( | |||
| ) | |||
|
|
|||
| companion object { | |||
| private const val BASE_DIR_NAME_REG_EX = "([a-z]+[-|_])+" | |||
| private const val BASE_DIR_NAME_REG_EX = "([a-z]+-)+" | |||
There was a problem hiding this comment.
I'm not sure if it is simpler, but probably it just can be [a-z\-]+. Yes, it allows to have multiple - or even only -, but maybe evaluation will be simpler/faster. I checked ([a-z]+-)+v[0-9]+ and for test string session-replay-v2 it captures 2 groups (well, not a bit deal, since overall string still matches).
Anyway, not blocking for me I think.
| @@ -608,13 +608,13 @@ internal class BatchMetricsDispatcherTest { | |||
| } | |||
|
|
|||
| private fun Forge.forgeAGrantedDirName(): String { | |||
| val separator = if (aBool()) "_" else "-" | |||
| val separator = "-" | |||
There was a problem hiding this comment.
can be just as joinToString argument like joinToString(separator = "-"). Same below
There was a problem hiding this comment.
yes I just wanted to keep that for visibility in case I need to update this later...sorry it's been so many changes here in this PR that I'd rather keep it like that.
Codecov Report
@@ Coverage Diff @@
## develop #1645 +/- ##
===========================================
+ Coverage 83.66% 83.72% +0.06%
===========================================
Files 456 456
Lines 15696 15696
Branches 2347 2347
===========================================
+ Hits 13131 13141 +10
+ Misses 1932 1928 -4
+ Partials 633 627 -6
|
mariusc83
force-pushed
the
mconstantin/fix-reg-ex-in-fetaure-file-orchestrator
branch
from
8b5140f to
0f130fc
Compare
mariusc83
deleted the
mconstantin/fix-reg-ex-in-fetaure-file-orchestrator
branch
What does this PR do?
The NDK folders do not produce batching telemetry so we had to simplify the rules a bit by removing the
|separator case.Motivation
What inspired you to submit this pull request?
Additional Notes
Anything else we should know when reviewing?
Review checklist (to be filled by reviewers)