chore: limiting dynamic config in zendesk #1796
Conversation
sanpj2292
requested review from
lvrach,
cisse21,
debanjan97,
am6010,
ssbeefeater,
ruchiramoitra,
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020,
1abhishekpandey,
RanjeetMishra,
saikumarrs and
a team
as code owners
WalkthroughThe changes involve significant updates to the Changes
Suggested reviewers
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (5)
src/configurations/destinations/zendesk/ui-config.json (1)
Line range hint
1-211: Consider a consistent strategy for dynamic configurationThe current changes show mixed approaches to dynamic configuration:
- Some fields are getting expanded dynamic config support (email, subdomain, source name)
- Other fields are getting limited through regex simplification (API token, consent fields)
Consider:
- Documenting the criteria for determining which fields should support dynamic configuration
- Maintaining consistent regex patterns across similar field types
- Adding validation rules in the schema to ensure proper value resolution when dynamic config is used
scripts/schemaGenerator.py (4)
27-28: Add comments to explain default regex patternsConsider adding comments to describe the purpose and usage of
defaultSubPatternanddefaultEnvPatternfor better code readability.
61-75: Clarify logical conditions inis_field_purposefunctionTo improve readability and ensure correct logic evaluation, add parentheses to make the precedence of
andandoroperations explicit.Apply this diff:
def is_field_purpose(field): """Checks if a field is a purpose field by looking for 'purpose' in either the 'value' or 'configKey' property. Args: field (object): Individual field in ui-config, containing properties like value or configKey. Returns: bool: True if field is a purpose field, False otherwise. """ return ( - "value" in field and field["value"] == "purpose" + ("value" in field and field["value"] == "purpose") or - "configKey" in field and field["configKey"] == "purpose" + ("configKey" in field and field["configKey"] == "purpose") )
76-95: Reduce code duplication ingenerate_for_regexed_fieldThe checks for
defaultSubPatternanddefaultEnvPatternare similar in both branches of theifstatement. Consider refactoring to reduce repetition and improve maintainability.You could combine the logic to handle
defaultSubPatternanddefaultEnvPatternin a single place.
96-108: Simplify pattern concatenation ingenerate_for_non_regexed_fieldThe logic for appending patterns can be streamlined for better readability.
Apply this diff:
def generate_for_non_regexed_field(field) -> str: # TODO: we should not use a case here for the individual properties. Just pass the desired pattern as regex property # in ketch purpose fields and delete next case pattern = "^(.{0,100})$" if is_field_purpose(field): return pattern # non purpose field & regex is not present in the field if field["dynamicConfigSupported"]: pattern = "|".join([defaultSubPattern, pattern]) - if defaultEnvPattern not in pattern: - pattern = "|".join([defaultEnvPattern, pattern]) + pattern = "|".join([defaultEnvPattern, pattern]) if defaultEnvPattern not in pattern else pattern return pattern
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
scripts/schemaGenerator.py(3 hunks)src/configurations/destinations/zendesk/schema.json(34 hunks)src/configurations/destinations/zendesk/ui-config.json(6 hunks)
🔇 Additional comments (9)
src/configurations/destinations/zendesk/ui-config.json (3)
20-20: LGTM! Security improvement in API token configuration
The simplified regex pattern that only allows environment variables improves security by restricting how API tokens can be configured. This is a positive change that aligns with best practices for handling sensitive credentials.
87-87: LGTM! Consistent regex pattern simplification
The regex pattern simplification across consent-related fields:
- Improves consistency in validation rules
- Properly limits configuration options to environment variables
- Aligns with the PR objective of limiting dynamic configuration
Also applies to: 115-115, 196-196
33-34: Review dynamic configuration strategy for subdomain and source name
Similar to the email field, adding dynamicConfigSupported: true to these fields appears to expand rather than limit dynamic configuration capabilities. Please clarify:
- Why are we enabling dynamic configuration for these fields?
- How does this align with the PR objective of limiting dynamic config?
- Should we maintain consistency in regex patterns across all fields?
#!/bin/bash
# Check the evolution of dynamic config support in this destination
git log -p src/configurations/destinations/zendesk/ui-config.json | rg -A 5 -B 5 "dynamicConfigSupported"Also applies to: 44-45
src/configurations/destinations/zendesk/schema.json (5)
50-50: Consistent pattern updates across OneTrust cookie categories
The regex pattern changes for oneTrustCookieCategory fields across all platforms (android, ios, web, unity, amp, cloud, warehouse, reactnative, flutter, cordova, shopify) maintain consistency in the validation rules. This is a good practice for maintainability.
Also applies to: 62-62, 74-74, 86-86, 98-98, 110-110, 122-122, 134-134, 146-146, 158-158, 170-170
197-197: Consistent pattern updates for consent management
The regex pattern changes for consent fields in the consent management section maintain consistency across all platforms. This aligns with the PR objective of limiting dynamic configuration.
Also applies to: 243-243, 289-289, 335-335, 381-381, 427-427, 473-473, 519-519, 565-565, 611-611, 657-657
747-747: Consistent pattern updates for Ketch consent purposes
The regex pattern changes for purpose fields in the Ketch consent purposes section maintain consistency across all platforms. This aligns with the PR objective of limiting dynamic configuration.
Also applies to: 759-759, 771-771, 783-783, 795-795, 807-807, 819-819, 831-831, 843-843, 855-855, 867-867
Line range hint 1-875: Verify impact on existing configurations
While the pattern changes are consistent and align with the PR objective, we should verify the impact on existing configurations and ensure proper migration documentation.
Let's analyze the usage patterns:
#!/bin/bash
# Description: Analyze current configuration patterns
# Expected results: Overview of current configuration usage patterns
# Check for environment variable usage
echo "Checking environment variable usage..."
rg -l '^env\.' | grep -i zendesk
# Check for direct value usage
echo "Checking direct value usage..."
rg -l '^\w{1,100}$' | grep -i zendesk
# Generate statistics for different patterns
echo "Generating pattern statistics..."
for pattern in 'env\.' '^\w{1,100}$' '\{\{.*\|\|.*\}\}'; do
echo "Pattern: $pattern"
rg -c "$pattern" | grep -i zendesk
done14-14:
Breaking Change: API Token pattern modification
The regex pattern for apiToken has been simplified to only allow environment variables or direct values, removing support for template variables with fallbacks ({{var||fallback}}). This change could break existing configurations that use template variables.
Let's verify if any existing configurations would be affected:
✅ Verification successful
Verification Successful: No existing Zendesk configurations use template variables.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for existing Zendesk configurations using template variables
# Expected results: List of configurations that might break with the new pattern
# Search for template variable pattern in JSON files
rg -l '\{\{.*\|\|.*\}\}' | grep -i zendesk
Length of output: 849
scripts/schemaGenerator.py (1)
123-124: Ensure consistent handling in generalize_regex_pattern
Verify that calling generate_for_regexed_field(field) when "regex" is in field aligns with the intended behavior, and that fields without "regex" are correctly processed by generate_for_non_regexed_field(field).
| "placeholder": "e.g: [email protected]", | ||
| "dynamicConfigSupported": true |
There was a problem hiding this comment.
💡 Codebase verification
Dynamic configuration support contradicts PR objectives
Adding "dynamicConfigSupported": true to multiple fields expands dynamic configuration contrary to the goal of limiting it in the Zendesk destination.
🔗 Analysis chain
Verify if email field should support dynamic configuration
Adding dynamicConfigSupported: true appears to expand rather than limit dynamic configuration capabilities. This seems to contradict the PR objective of "limiting dynamic config in zendesk". Additionally, the regex pattern above still includes template placeholders ({{..||..}}), which is inconsistent with other regex simplifications in this PR.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if other destination configs have similar dynamic config support for email
rg -l '"dynamicConfigSupported":\s*true' src/configurations/destinations/*/ui-config.json
Length of output: 1463
- we are considering these scenarios {{a.v.c||laksdn}}skiv, skiv{{a.v.c||laksdn}}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (4)
src/configurations/destinations/zendesk/ui-config.json (1)
Line range hint
1-211: Architectural inconsistency in dynamic configuration approachThere appears to be a fundamental inconsistency in how dynamic configuration is being handled:
- Connection Settings fields (email, subdomain, source name) are being made more dynamic by adding
dynamicConfigSupported: true- Consent Settings fields are being made less dynamic through regex restrictions
This mixed approach seems to contradict the PR's objective of "limiting dynamic config in zendesk". Consider:
- Removing the new
dynamicConfigSupportedflags to align with the PR objective- Applying consistent regex restrictions across all fields that should have limited dynamic configuration
src/configurations/destinations/zendesk/schema.json (3)
50-50: Pattern Update: OneTrust Cookie Categories across platformsThe pattern changes for
oneTrustCookieCategoryfields across all platforms follow the same security improvement as the API token, preventing template variables with fallbacks.This consistent pattern application across platforms ensures uniform behavior and security constraints.
Also applies to: 62-62, 74-74, 86-86, 98-98, 110-110, 122-122, 134-134, 146-146, 158-158, 170-170
197-197: Pattern Update: Consent Management fields across platformsThe pattern changes for consent fields across all platforms align with the security improvements, maintaining consistency with other field updates.
The uniform pattern application ensures consistent validation across all platforms and integration points.
Also applies to: 243-243, 289-289, 335-335, 381-381, 427-427, 473-473, 519-519, 565-565, 611-611, 657-657
Line range hint
1-875: Overall Schema Structure AssessmentThe schema maintains a consistent structure and validation patterns across all platforms and field types. The changes effectively implement the PR objective of limiting dynamic configuration while preserving environment variable support.
Consider documenting these validation changes in the migration guide to help users transition from template variables to environment variables.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
src/configurations/destinations/zendesk/schema.json(34 hunks)src/configurations/destinations/zendesk/ui-config.json(6 hunks)
🔇 Additional comments (5)
src/configurations/destinations/zendesk/ui-config.json (3)
20-20: Appropriate restriction of template placeholders
The updated regex pattern correctly limits dynamic configuration by preventing template placeholders while still allowing environment variables. This aligns well with the PR objectives.
87-87: Consistent and appropriate regex restrictions
The regex pattern updates across consent-related fields (oneTrustCookieCategory, purpose, and consent) consistently prevent template placeholders while maintaining support for environment variables. This implementation aligns well with the goal of limiting dynamic configuration.
Also applies to: 115-115, 196-196
10-14:
Dynamic configuration support contradicts PR objectives
Adding "dynamicConfigSupported": true appears to expand rather than limit dynamic configuration capabilities. This seems to contradict the PR objective of "limiting dynamic config in zendesk".
src/configurations/destinations/zendesk/schema.json (2)
747-747: Pattern Update: Ketch Consent Purposes across platforms
The pattern changes for purpose fields in Ketch consent management follow the same security improvements as other fields.
Also applies to: 759-759, 771-771, 783-783, 795-795, 807-807, 819-819, 831-831, 843-843, 855-855, 867-867
✅ Verification successful
Based on the search results, I can see that the pattern change for purpose fields is consistent with test data across multiple destination configurations. All test files show the same template variable usage pattern: "purpose": "{{ event.properties.prop1 || 'val' }}". Let me verify the actual schema change in the Zendesk configuration.
Let me try a different approach to verify the pattern change in the Zendesk schema file.
Let me try one more time with a simpler pattern to check the schema file.
Pattern Update for Ketch Consent Purposes is Consistent
The pattern change "(^env[.].+)|^(?!.*\\{\\{.*\\|\\|(.*)\\}\\}).{0,100}$" is consistently applied across all purpose fields in the Zendesk schema, matching the security improvements. This pattern:
- Allows environment variables (
env.prefix) - Prevents template variables with fallback syntax (
{{...||...}}) - Limits string length to 100 characters
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for any existing configurations using template variables in purpose fields
rg -l '"purpose":\s*".*\{\{.*\|\|.*\}\}"' | while read -r file; do
echo "Found template variable usage in $file:"
rg -A 1 '"purpose":\s*".*\{\{.*\|\|.*\}\}"' "$file"
done
Length of output: 82846
Script:
#!/bin/bash
# Check the pattern in the Zendesk schema file
rg -A 1 '"pattern":\s*".*env.*\|\|.*"' src/configurations/destinations/zendesk/schema.json
Length of output: 92
Script:
#!/bin/bash
# Check the specific line and surrounding context in the Zendesk schema file
rg -C 5 '(^env[.].+)|^(?!.*\{\{.*\|\|(.*)}}' src/configurations/destinations/zendesk/schema.json
Length of output: 349
Script:
#!/bin/bash
# Get the content around line 747 in the Zendesk schema file
rg -C 5 -n '"pattern":' src/configurations/destinations/zendesk/schema.json
Length of output: 13522
14-14:
Breaking Change: API Token pattern restricts template variables
The new pattern prevents the use of template variables with fallbacks ({{var||fallback}}), while still allowing environment variables. This is a breaking change that improves security by limiting dynamic configuration.
✅ Verification successful
Based on the test data content, I can now generate the final response:
Pattern change improves security without breaking existing configurations
The new pattern (^env[.].+)|^(?!.*\{\{.*\|\|(.*)}}}).{1,100}$ correctly allows environment variables while preventing template variables with fallbacks. The test data shows that:
- All test cases use direct string values or environment variables for
apiToken - No test cases rely on template variables with fallbacks for
apiToken - The only template variables with fallbacks appear in optional consent fields, not in critical authentication fields
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for existing configurations using template variables in apiToken
rg -l '"apiToken":\s*".*\{\{.*\|\|.*\}\}"' | while read -r file; do
echo "Found template variable usage in $file:"
rg -A 1 '"apiToken":\s*".*\{\{.*\|\|.*\}\}"' "$file"
done
Length of output: 59
Script:
#!/bin/bash
# Let's try a broader search for any configuration files that might use template variables
# First, find all JSON files that might contain Zendesk configurations
fd -e json | while read -r file; do
if grep -q "zendesk" "$file"; then
echo "=== $file ==="
# Look for any template variable pattern in the file
grep -A 1 '\{\{.*||.*\}\}' "$file" || true
fi
done
# Also search for any documentation or example files that might show template usage
rg -l --type md --type txt '\{\{.*\|\|.*\}\}' || true
Length of output: 100382
Script:
#!/bin/bash
# Let's also check if there are any tests that might be using template variables
fd -e test.js -e spec.js | while read -r file; do
if grep -q "zendesk" "$file"; then
echo "=== $file ==="
# Look for template variable patterns in test files
grep -A 1 '\{\{.*||.*\}\}' "$file" || true
fi
done
Length of output: 41
Script:
#!/bin/bash
# Let's check the git history for any recent changes related to template variables in Zendesk configs
git log -n 5 --oneline --grep="zendesk" --grep="template" || true
Length of output: 0
Script:
#!/bin/bash
# Let's check for any existing configurations in the test data that might be affected
rg -l "apiToken.*{{" test/data/validation/destinations/zendesk.json || true
# Also check for any documentation of template variables usage
rg -l "{{.*\|\|.*}}" "**/*.md" || true
# Let's also look at the schema history specifically
git log -p -n 1 src/configurations/destinations/zendesk/schema.json || true
Length of output: 310
Script:
#!/bin/bash
# Let's check the actual test data content
if [ -f "test/data/validation/destinations/zendesk.json" ]; then
echo "=== test/data/validation/destinations/zendesk.json ==="
cat test/data/validation/destinations/zendesk.json
fi
Length of output: 11448
| "placeholder": "Subdomain only not including .zendesk.com", | ||
| "dynamicConfigSupported": true |
There was a problem hiding this comment.
Inconsistent with dynamic config limitation goal
Adding dynamicConfigSupported: true to both Subdomain and Source Name fields continues the pattern of expanding rather than limiting dynamic configuration. This appears to be inconsistent with the PR's stated objective.
Also applies to: 44-45
|
This PR is considered to be stale. It has been open for 20 days with no further activity thus it is going to be closed in 7 days. To avoid such a case please consider removing the stale label manually or add a comment to the PR. |
What are the changes introduced in this PR?
Limit dynamic config to specific fields for Zendesk destination
What is the related Linear task?
Resolves INT-2902
Please explain the objectives of your changes below
Put down any required details on the broader aspect of your changes. If there are any dependent changes, mandatorily mention them here
Any changes to existing capabilities/behaviour, mention the reason & what are the changes ?
N/A
Any new dependencies introduced with this change?
N/A
Any new checks got introduced or modified in test suites. Please explain the changes.
N/A
Developer checklist
My code follows the style guidelines of this project
No breaking changes are being introduced.
All related docs linked with the PR?
All changes manually tested?
Any documentation changes needed with this change?
I have executed schemaGenerator tests and updated schema if needed
Are sensitive fields marked as secret in definition config?
My test cases and placeholders use only masked/sample values for sensitive fields
Is the PR limited to 10 file changes & one task?
Reviewer checklist
Is the type of change in the PR title appropriate as per the changes?
Verified that there are no credentials or confidential data exposed with the changes.
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Documentation