-
Notifications
You must be signed in to change notification settings - Fork 73
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
Fixes #12106: Implement general parameters for policy server policy c… #1857
Fixes #12106: Implement general parameters for policy server policy c… #1857
Conversation
@@ -551,7 +615,12 @@ <h3>Protocol</h3> | |||
</div> | |||
</div> | |||
</div> | |||
<<<<<<< HEAD |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
there is a merge problem here
private[this] implicit def toRelaySynchronisationMethod(p: RudderWebProperty): RelaySynchronizationMethod = p.value match { | ||
case ClassicSynchronization.value => ClassicSynchronization | ||
case RsyncSynchronization.value => RsyncSynchronization | ||
case DisabledSynchronization.value => DisabledSynchronization |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Heren the match is not exhaustive. If the value is not recognized, it will fails.
I thing we want to have "classic" as the default, so use:
case RsyncSynchronization.value => RsyncSynchronization
case DisabledSynchronization.value => DisabledSynchronization
case _ => ClassicSynchronization
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
you are totally right
@@ -188,6 +193,10 @@ object TestSystemData { | |||
//denybadclocks and skipIdentify are runtime properties | |||
, getDenyBadClocks = () => Full(true) | |||
, getSkipIdentify = () => Full(false) | |||
// relay synchronisation method | |||
, getSyncMethod = () => Full(ClassicSynchronization) | |||
, getSyncPromises = () => Full(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does it makes sense (from the feature behavior) to have separated items for sync method / sync promises / sync shared file ? The two last seems to have a meaning only for syncMethod=rsync, no ? (really asking, I don't have any idea from the ticket description about what should be the behavior)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
for the moment they have, but it could change in the future - to have rsync for one, and classic for another for instance
I didn't want to go the json path for something that were bound to change , and I prefered to stricly match the form and the system variable
Ho, you also need to update the API for settings to handle the new properties |
6790104
to
3405fa6
Compare
Commit modified |
3405fa6
to
d42f740
Compare
Commit modified |
This PR is not mergeable to upper versions. |
OK, merging this PR |
OK, merging this PR |
1 similar comment
OK, merging this PR |
…opy for nodes