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

feat: onboard Amazon Audience #1667

Merged
merged 11 commits into from
Oct 9, 2024
Merged

Conversation

anantjain45823
Copy link
Contributor

@anantjain45823 anantjain45823 commented Sep 9, 2024

What are the changes introduced in this PR?

Onboard new destination Amazon Audience.
Supporting Blank Audience, Dynamic Fields and VDM suppport and Mirror mode with record type of events only

Write a brief explainer on your code changes.

What is the related Linear task?

Resolves INT-2604

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

  • New Features

    • Introduced a new configuration for the Amazon Audience destination, including settings for authentication and data synchronization.
    • Added a user interface configuration file for easier setup and management of Amazon Audience settings.
    • Implemented a JSON schema for validating Amazon Audience configurations.
  • Tests

    • Added test cases to validate Amazon Audience configuration settings, ensuring proper handling of required fields.
  • Documentation

    • Included notes in the configuration to guide users on finding the Account ID.

Copy link

codecov bot commented Sep 9, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 100.00%. Comparing base (130aae2) to head (eaa93d3).
Report is 83 commits behind head on release/v1.95.0.

Additional details and impacted files
@@                Coverage Diff                @@
##           release/v1.95.0     #1667   +/-   ##
=================================================
  Coverage           100.00%   100.00%           
=================================================
  Files                    2         2           
  Lines                   53        53           
  Branches                 7         7           
=================================================
  Hits                    53        53           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…ig.json

Co-authored-by: Yashasvi Bajpai <33063622+yashasvibajpai@users.noreply.github.com>
utsabc
utsabc previously approved these changes Sep 26, 2024
Gauravudia
Gauravudia previously approved these changes Oct 8, 2024
Copy link

coderabbitai bot commented Oct 9, 2024

Walkthrough

A new configuration for the Amazon Audience destination has been introduced, including three new JSON files: db-config.json, schema.json, and ui-config.json, which define settings, validation rules, and user interface configurations, respectively. Additionally, a note was added to the existing ui-config.json for the x_audience destination. Test cases for validating the Amazon Audience configurations have also been added in a new JSON file.

Changes

File Path Change Summary
src/configurations/destinations/amazon_audience/db-config.json New configuration file for Amazon Audience with properties for integration settings.
src/configurations/destinations/amazon_audience/schema.json New JSON schema file defining structure and validation rules for Amazon Audience configuration.
src/configurations/destinations/amazon_audience/ui-config.json New UI configuration file detailing settings for Amazon Audience destination.
src/configurations/destinations/x_audience/ui-config.json Added note to "accountId" field in x_audience configuration.
test/data/validation/destinations/amazon_audience.json New test cases for validating Amazon Audience configurations, including complete and incomplete cases.

Possibly related PRs

  • chore: adding salesforce oauth sandbox support for closed testing #1732: The changes in this PR involve the introduction of a new configuration file and schema for the Salesforce OAuth Sandbox, similar to the new configuration files and schema added for the Amazon Audience destination in the main PR. Both PRs focus on defining properties and validation rules for their respective integrations.

Suggested reviewers

  • lvrach
  • am6010
  • AchuthaSourabhC
  • krishna2020
  • ssbeefeater
  • debanjan97
  • cisse21
  • utsabc
  • ruchiramoitra
  • koladilip
  • 1abhishekpandey
  • ItsSudip
  • sandeepdsvs
  • sanpj2292

Poem

In the garden of code where the changes bloom,
New configs sprout, dispelling the gloom.
Amazon Audience joins the merry dance,
With schemas and tests, we give it a chance.
Hoppity hop, let the validations sing,
For every new feature, joy they will bring! 🐇✨


📜 Recent review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between e357dbc and 941cf61.

📒 Files selected for processing (3)
  • src/configurations/destinations/amazon_audience/db-config.json (1 hunks)
  • src/configurations/destinations/amazon_audience/schema.json (1 hunks)
  • src/configurations/destinations/amazon_audience/ui-config.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
  • src/configurations/destinations/amazon_audience/db-config.json
  • src/configurations/destinations/amazon_audience/schema.json
  • src/configurations/destinations/amazon_audience/ui-config.json

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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🧹 Outside diff range and nitpick comments (7)
test/data/validation/destinations/amazon_audience.json (3)

1-20: LGTM! Consider type consistency for ttl.

The test case "With all required configs" looks good and covers all the necessary fields for Amazon Audience configuration. However, there's a minor suggestion:

Consider changing the ttl value from a string to a number for consistency:

-      "ttl": "1230",
+      "ttl": 1230,

This change would ensure type consistency if the ttl is expected to be a numeric value in the actual implementation.


21-32: Approve with suggestions for clarity and completeness.

The test case "With missing accountId" effectively tests the validation failure scenario. However, there are a few suggestions to improve clarity and completeness:

  1. The test title doesn't accurately reflect the test case. Consider renaming it to "With missing advertiserId" for clarity.

  2. To make the test more robust, consider adding other required fields that are present in the successful test case.

  3. The error message suggests a specific pattern for advertiserId. It might be helpful to include a test case that validates this pattern.

Here's a suggested improvement:

-    "testTitle": "With missing accountId",
+    "testTitle": "With missing advertiserId",
     "config": {
       "rudderAccountId": "test-host",
+      "ttl": 1230,
+      "enableHash": true,
+      "dataSourceCountry": [
+        {
+          "country": "US"
+        }
+      ],
       "audienceId": "test-audience-id",
       "advertiserId": ""
     },

Would you like me to generate additional test cases to cover the advertiserId pattern validation?


1-33: Consider adding more test cases for comprehensive coverage.

The current test cases provide a good starting point for validating Amazon Audience configurations. To ensure robust validation, consider adding the following test cases:

  1. Test with minimum required fields only.
  2. Test with invalid dataSourceCountry values.
  3. Test with invalid ttl values (e.g., negative numbers, non-numeric strings).
  4. Test with different valid patterns for advertiserId.
  5. Test with all fields present but invalid audienceId.

These additional test cases will help ensure that the validation logic handles various edge cases and provides comprehensive coverage of the configuration space.

Would you like assistance in generating these additional test cases?

src/configurations/destinations/amazon_audience/schema.json (3)

7-11: LGTM: "authServer" property is correctly defined.

The "authServer" property is properly defined as a string with the only allowed value being "North America". The default value is also set correctly.

Consider using a more flexible enum if there's a possibility of supporting additional auth servers in the future. This would make the schema more adaptable to potential changes without requiring modifications.


12-23: LGTM: String properties are well-defined with appropriate validation patterns.

The "advertiserId", "externalAudienceId", and "ttl" properties are correctly defined as strings with regex patterns that allow for template variables, environment variables, and direct values. This flexibility is good for configuration management.

For the "ttl" property, consider adding a "description" field to clarify the expected time unit (e.g., seconds, minutes, hours) for the TTL value. This would improve the schema's self-documentation.


24-39: LGTM: "enableHash" and "dataSourceCountry" properties are well-structured.

The "enableHash" boolean property and the "dataSourceCountry" array property are correctly defined. The flexibility in the "country" property's regex pattern is consistent with other string properties in the schema.

For the "dataSourceCountry" property, consider adding a "minItems" constraint if there should be at least one country specified. Also, you might want to add a "uniqueItems" constraint to ensure no duplicate country entries.

src/configurations/destinations/x_audience/ui-config.json (1)

Unrelated changes detected: The modifications to src/configurations/destinations/x_audience/ui-config.json appear unrelated to the Amazon Audience integration. Please confirm if this change is intentional as part of this PR or if it should be moved to a separate improvement.

🔗 Analysis chain

Line range hint 1-91: Verify the relevance of this change to the PR objectives.

The PR objectives mention introducing a new destination integration for Amazon Audience, but this file is for the X Audience (formerly Twitter) destination. Could you please clarify if this change is intended to be part of this PR, or if it's an unrelated improvement?

To help verify this, you can run the following script:

This script will help us understand if there are indeed Amazon Audience related changes in this PR, and if the X Audience change is part of a separate improvement.

🏁 Scripts executed

The following scripts were executed for the analysis:

Script:

#!/bin/bash
# Description: Check for Amazon Audience related files and changes

# Test 1: Check if there are new Amazon Audience configuration files
echo "Checking for new Amazon Audience configuration files:"
fd -e json 'amazon_audience'

# Test 2: Check for mentions of Amazon Audience in recent commits
echo "Checking for mentions of Amazon Audience in recent commits:"
git log -n 5 --oneline --grep="Amazon Audience"

# Test 3: Check for mentions of X Audience in recent commits
echo "Checking for mentions of X Audience in recent commits:"
git log -n 5 --oneline --grep="X Audience"

Length of output: 581

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between b2a433b and 4972d80.

📒 Files selected for processing (5)
  • src/configurations/destinations/amazon_audience/db-config.json (1 hunks)
  • src/configurations/destinations/amazon_audience/schema.json (1 hunks)
  • src/configurations/destinations/amazon_audience/ui-config.json (1 hunks)
  • src/configurations/destinations/x_audience/ui-config.json (1 hunks)
  • test/data/validation/destinations/amazon_audience.json (1 hunks)
🧰 Additional context used
🔇 Additional comments (11)
src/configurations/destinations/amazon_audience/db-config.json (4)

1-7: Basic configuration looks good, consider reviewing excludeKeys.

The basic configuration for the Amazon Audience destination is well-structured. However, the excludeKeys array is empty. Please review if there are any keys that should be excluded from processing.


12-16: Audience and sync configuration looks good. Please clarify mapper choices.

The audience support and sync configuration align well with the PR objectives. The choice to disable the JSON mapper while enabling the visual mapper is interesting. Could you provide more context on why this decision was made?


17-31: Destination configuration is comprehensive. Review secretKeys if needed.

The destination configuration includes all necessary fields and aligns with the expected structure for Amazon Audience. However, the secretKeys array is empty. Please review if there are any secret keys that should be included for this integration.

Also, note that only the "record" message type is supported for cloud connections. Confirm if this is the intended configuration.


32-34: Beta status is correctly set.

The configuration is appropriately marked as beta, which aligns with the introduction of this new integration.

src/configurations/destinations/amazon_audience/schema.json (3)

1-6: LGTM: Schema definition and root object structure are correct.

The schema is properly defined using JSON Schema draft-07, and the root object is correctly specified as an object type with the required "authServer" property.


1-49: Overall, the Amazon Audience configuration schema is well-defined and meets the requirements.

The schema provides a comprehensive structure for the Amazon Audience configuration, including all necessary properties with appropriate validation rules. The use of regex patterns for string properties allows for flexible configuration management, including support for template variables and environment variables.

Key points:

  1. The schema structure aligns with the PR objectives for the Amazon Audience integration.
  2. Validation rules are consistent and well-defined across properties.
  3. The schema allows for future extensibility in some areas (e.g., potential additional auth servers).

Minor suggestions for improvement have been made in previous comments, mainly focusing on adding constraints or clarifications to enhance the schema's robustness and self-documentation.


40-46: Please clarify the purpose of the "connectionMode" property structure.

The "connectionMode" object is correctly defined with "cloud" and "warehouse" properties. However, both properties have the same single allowed value "cloud".

Could you please clarify the intention behind having two separate properties with identical constraints? If this is for future expansion, consider adding a comment in the schema to explain the purpose. If not, it might be more straightforward to have a single "connectionMode" string property.

To verify the usage of this property in the codebase, you can run the following script:

This will help us understand how the property is used in other parts of the configuration.

src/configurations/destinations/x_audience/ui-config.json (1)

20-20: LGTM: Helpful note added for Account ID location.

The addition of this note provides clear guidance to users on where to find the Account ID. This improves the user experience and reduces potential confusion.

src/configurations/destinations/amazon_audience/ui-config.json (3)

34-34: Update the authorization servers note.

As suggested in a previous review, please update the note for clarity.

Apply this change:

-                    "note": " Support for more servers will be added soon",
+                    "note": "Support for more servers will be added soon",

1-150: Overall structure and content look good.

The configuration file is well-structured and aligns with the PR objectives of introducing Amazon Audience integration. It includes all the necessary sections and fields as described in the PR summary and AI-generated overview.


2-2: ⚠️ Potential issue

Remove the generated code comment.

This line appears to be a remnant from a template and doesn't accurately reflect the nature of this configuration file. As suggested in a previous review, please remove this line.

Apply this change:

-  "description": "GENERATED CODE DO NOT EDIT",

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
src/configurations/destinations/amazon_audience/schema.json (5)

1-11: Consider future extensibility for the authServer property.

The authServer property is currently limited to only "North America". While this might be sufficient for the current implementation, it could potentially limit future expansion to other regions.

Consider if there's a possibility of expanding to other regions in the future. If so, it might be beneficial to design the schema to accommodate potential additions without breaking changes.


12-23: LGTM! Consider standardizing regex patterns for consistency.

The advertiserId, externalAudienceId, and ttl properties are well-defined with flexible regex patterns that allow for template variables, environment variables, and specific formats. This approach provides good validation while maintaining flexibility for various use cases.

For improved consistency, consider standardizing the regex patterns across all string properties in the schema. For example, the pattern for ttl could be updated to match the format of the other properties:

-        "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^[0-9]\\d*$"
+        "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^([0-9]\\d*)$"

This change wraps the number pattern in parentheses, making it consistent with the other properties' patterns.


28-51: LGTM! Consider adding a description for clarity.

The dataSourceCountry and oneTrustCookieCategories properties are well-structured as arrays of objects, allowing for multiple entries. The regex patterns are consistent with earlier properties, providing flexibility for various use cases.

To improve clarity, consider adding a description field to these properties. For example:

   "dataSourceCountry": {
     "type": "array",
+    "description": "List of countries associated with the data source",
     "items": {
       // ... existing code ...
     }
   },
   "oneTrustCookieCategories": {
     "type": "array",
+    "description": "List of OneTrust cookie categories",
     "items": {
       // ... existing code ...
     }
   }

This addition would provide more context about the purpose of these properties to users of the schema.


52-58: Consider future extensibility for connectionMode properties.

The connectionMode object is well-structured, allowing for different connection modes for cloud and warehouse environments. However, both properties currently only allow "cloud" as a value.

If there's a possibility of supporting other connection modes in the future (e.g., "on-premise"), consider making the schema more flexible. You could either:

  1. Use an enum with multiple values:

    "cloud": { "type": "string", "enum": ["cloud", "on-premise"] },
    "warehouse": { "type": "string", "enum": ["cloud", "on-premise"] }
  2. Or use a pattern to allow for future expansion:

    "cloud": { "type": "string", "pattern": "^[a-zA-Z0-9-_]+$" },
    "warehouse": { "type": "string", "pattern": "^[a-zA-Z0-9-_]+$" }

This would make the schema more adaptable to future requirements without necessitating breaking changes.


1-61: Overall, the schema is well-designed and aligns with the PR objectives.

This new schema for the Amazon Audience configuration is comprehensive and well-structured. It provides robust validation rules for all necessary properties, including support for authentication, audience identification, and connection modes as outlined in the PR objectives.

Key strengths of the schema:

  1. Flexibility through the use of regex patterns, allowing for template and environment variables.
  2. Appropriate use of default values and required fields.
  3. Support for multiple data source countries and OneTrust cookie categories.

The schema successfully introduces the new destination integration for Amazon Audience, supporting the features mentioned in the PR summary, such as Blank Audience and Dynamic Fields.

As the integration evolves, keep in mind the suggestions for future extensibility, particularly for the authServer and connectionMode properties. This will ensure the schema remains adaptable to potential future requirements without necessitating breaking changes.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 4972d80 and 3bd84a5.

📒 Files selected for processing (3)
  • src/configurations/destinations/amazon_audience/db-config.json (1 hunks)
  • src/configurations/destinations/amazon_audience/schema.json (1 hunks)
  • src/configurations/destinations/amazon_audience/ui-config.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/configurations/destinations/amazon_audience/db-config.json
  • src/configurations/destinations/amazon_audience/ui-config.json
🧰 Additional context used
🔇 Additional comments (1)
src/configurations/destinations/amazon_audience/schema.json (1)

24-27: LGTM! The enableHash property is well-defined.

The enableHash property is correctly defined as a boolean with a default value of false. This is appropriate for an opt-in feature.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (5)
src/configurations/destinations/amazon_audience/schema.json (5)

1-11: LGTM! Consider future extensibility for authServer.

The root object and authServer property are well-defined. The default value for authServer is a good usability feature.

Consider if the authServer might need to support other regions in the future. If so, it may be beneficial to design the schema to allow for easy addition of new regions.


24-51: LGTM! Consider adding 'required' property for consistency.

The enableHash, dataSourceCountry, and oneTrustCookieCategories properties are well-defined. The array structures allow for multiple entries with flexible validation patterns.

For consistency, consider adding a 'required' property to the object definitions within dataSourceCountry and oneTrustCookieCategories arrays. For example:

 "dataSourceCountry": {
   "type": "array",
   "items": {
     "type": "object",
+    "required": ["country"],
     "properties": {
       "country": {
         "type": "string",
         "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{0,100})$"
       }
     }
   }
 },

Apply a similar change to the oneTrustCookieCategories array.


52-102: LGTM! Well-structured consent management configuration.

The consentManagement property is well-defined with appropriate nested structures and conditional logic. The use of enums and the if-then construct ensures data integrity and proper configuration based on the provider type.

For improved clarity, consider adding a description to the resolutionStrategy property explaining its purpose and when it's required. For example:

 "resolutionStrategy": {
   "type": "string",
   "enum": ["and", "or"],
+  "description": "Specifies how multiple consents should be combined. Required only when the provider is 'custom'."
 }

103-109: Consider future extensibility and clarify the purpose of connectionMode properties.

The connectionMode property structure is simple and well-defined. However, there are a few points to consider:

  1. Future Extensibility: Both cloud and warehouse properties currently only allow the value "cloud". Consider if there might be other connection modes in the future. If so, it may be beneficial to design the schema to allow for easy addition of new modes.

  2. Redundancy: The cloud and warehouse properties are currently identical. If they serve different purposes, consider adding descriptions to clarify their roles. If they are truly identical, consider simplifying to a single property.

Example of adding descriptions for clarity:

 "connectionMode": {
   "type": "object",
   "properties": {
-    "cloud": { "type": "string", "enum": ["cloud"] },
-    "warehouse": { "type": "string", "enum": ["cloud"] }
+    "cloud": { 
+      "type": "string", 
+      "enum": ["cloud"],
+      "description": "Specifies the connection mode for cloud operations."
+    },
+    "warehouse": { 
+      "type": "string", 
+      "enum": ["cloud"],
+      "description": "Specifies the connection mode for warehouse operations."
+    }
   }
 }

If the properties are truly identical and serve the same purpose, consider simplifying:

 "connectionMode": {
-  "type": "object",
-  "properties": {
-    "cloud": { "type": "string", "enum": ["cloud"] },
-    "warehouse": { "type": "string", "enum": ["cloud"] }
-  }
+  "type": "string",
+  "enum": ["cloud"],
+  "description": "Specifies the connection mode for both cloud and warehouse operations."
 }

1-112: Excellent schema structure with comprehensive validation.

The overall schema for the Amazon Audience configuration is well-designed and comprehensive. It covers various aspects of the configuration with appropriate validation rules, making good use of JSON Schema features like conditional validation and regex patterns.

To further improve the schema, consider adding a top-level description to provide an overview of the configuration's purpose and any important notes for users. For example:

 {
   "configSchema": {
     "$schema": "http://json-schema.org/draft-07/schema#",
+    "description": "Configuration schema for Amazon Audience integration. This schema defines the structure and validation rules for setting up the Amazon Audience destination, including authentication, audience management, consent handling, and connection modes.",
     "type": "object",
     // ... rest of the schema
   }
 }

This addition would provide valuable context for developers working with this configuration.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 3bd84a5 and 278b35a.

📒 Files selected for processing (3)
  • src/configurations/destinations/amazon_audience/db-config.json (1 hunks)
  • src/configurations/destinations/amazon_audience/schema.json (1 hunks)
  • src/configurations/destinations/amazon_audience/ui-config.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/configurations/destinations/amazon_audience/db-config.json
  • src/configurations/destinations/amazon_audience/ui-config.json
🧰 Additional context used
🔇 Additional comments (1)
src/configurations/destinations/amazon_audience/schema.json (1)

12-23: LGTM! Well-defined properties with flexible validation.

The advertiserId, externalAudienceId, and ttl properties are well-defined with appropriate regex patterns. These patterns allow for template variables, environment variables, and specific formats, providing flexibility while maintaining data integrity.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Outside diff range and nitpick comments (7)
src/configurations/destinations/amazon_audience/schema.json (7)

1-11: LGTM! Consider future extensibility for authServer.

The root structure and authServer property are correctly defined. However, having only one option ("North America") for authServer might limit future extensibility. Consider if there are plans to expand to other regions in the future.

If expansion to other regions is planned, consider changing the enum to an array of strings that can be easily extended, e.g., ["North America", "Europe", "Asia"].


12-23: LGTM! Consider adding property descriptions.

The advertiserId, externalAudienceId, and ttl properties are correctly defined with appropriate regex patterns. These patterns allow for template variables, environment variables, and specific formats, providing flexibility while maintaining constraints.

Consider adding descriptions for each property to improve schema readability and provide context for users. For example:

"advertiserId": {
  "type": "string",
  "pattern": "(^\\{\\{.*\\|\\|(.*)\\}\\}$)|(^env[.].+)|^(.{1,150})$",
  "description": "The unique identifier for the advertiser. Can be a template variable, environment variable, or up to 150 characters."
},

24-39: LGTM! Consider adding a description for enableHash.

The enableHash and dataSourceCountry properties are correctly defined. The dataSourceCountry structure allows for multiple country entries, providing flexibility.

Consider adding a description for the enableHash property to clarify its purpose. For example:

"enableHash": {
  "type": "boolean",
  "default": false,
  "description": "Enable hashing for sensitive data (if applicable)."
},

40-102: LGTM! Consider adding descriptions for consent-related properties.

The oneTrustCookieCategories and consentManagement properties are correctly defined with appropriate structures to handle various consent management scenarios. The conditional schema for the custom provider is well-implemented.

Consider adding descriptions for these consent-related properties to improve clarity. For example:

"oneTrustCookieCategories": {
  "type": "array",
  "description": "List of OneTrust cookie categories for consent management.",
  "items": {
    // ... existing structure ...
  }
},
"consentManagement": {
  "type": "object",
  "description": "Configuration for consent management across different providers.",
  "properties": {
    // ... existing structure ...
  }
},

103-119: LGTM! Consider adding a description for ketchConsentPurposes.

The ketchConsentPurposes property is correctly defined with a flexible structure allowing for multiple consent purposes. The regex pattern for the purpose property is consistent with other string properties in the schema.

Consider adding a description for the ketchConsentPurposes property to provide context. For example:

"ketchConsentPurposes": {
  "type": "object",
  "description": "Configuration for Ketch-specific consent purposes.",
  "properties": {
    // ... existing structure ...
  }
},

120-126: Consider simplifying connectionMode and future extensibility.

The connectionMode property is correctly defined, but there are a couple of points to consider:

  1. Both cloud and warehouse properties have only one option: "cloud". This might limit future extensibility if there are plans to support other connection modes.
  2. It's unclear why both properties are needed if they always have the same value.

Consider the following suggestions:

  1. If different connection modes are planned for the future, consider expanding the enum to include potential future options, e.g., ["cloud", "on-premise"].
  2. If cloud and warehouse will always have the same value, consider simplifying to a single property:
"connectionMode": {
  "type": "string",
  "enum": ["cloud"],
  "description": "The connection mode for both cloud and warehouse operations."
},

If different modes for cloud and warehouse are possible in the future, keep the current structure but add a description explaining the purpose of each.


1-129: Overall, well-structured and comprehensive schema.

The schema for the Amazon Audience configuration is well-defined and covers various aspects including authentication, advertising, consent management, and connection modes. It provides good validation rules for the configuration object.

Consider the following suggestions for further improvement:

  1. Add descriptions to all major properties to enhance schema readability and provide context for users.
  2. Review properties with limited options (e.g., authServer, connectionMode) to ensure they accommodate future extensibility if needed.
  3. Consider adding a version property to the schema to facilitate future updates and backward compatibility.
  4. If applicable, add examples for complex properties to guide users in correctly formatting their configuration.

Example of adding a version and examples:

{
  "configSchema": {
    "$schema": "http://json-schema.org/draft-07/schema#",
    "type": "object",
    "required": ["authServer"],
    "properties": {
      "schemaVersion": {
        "type": "string",
        "const": "1.0.0",
        "description": "Version of the Amazon Audience configuration schema"
      },
      // ... existing properties ...
    },
    "examples": [
      {
        "authServer": "North America",
        "advertiserId": "12345",
        "externalAudienceId": "audience-123",
        "ttl": "3600",
        "enableHash": true,
        "dataSourceCountry": [{"country": "US"}],
        // ... other properties ...
      }
    ]
  }
}

These additions would make the schema more robust and user-friendly.

📜 Review details

Configuration used: CodeRabbit UI
Review profile: CHILL

📥 Commits

Files that changed from the base of the PR and between 278b35a and e357dbc.

📒 Files selected for processing (3)
  • src/configurations/destinations/amazon_audience/db-config.json (1 hunks)
  • src/configurations/destinations/amazon_audience/schema.json (1 hunks)
  • src/configurations/destinations/amazon_audience/ui-config.json (1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
  • src/configurations/destinations/amazon_audience/db-config.json
  • src/configurations/destinations/amazon_audience/ui-config.json
🧰 Additional context used

"configSchema": {
"$schema": "http://json-schema.org/draft-07/schema#",
"type": "object",
"required": ["authServer"],
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't authServer also be required field ? It's in Initial setup

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah authServer should be required field hence it is kept in required array here

@anantjain45823 anantjain45823 changed the base branch from develop to release/v1.95.0 October 9, 2024 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants