chore: add item name to script to update single destination #1837
Conversation
koladilip
requested review from
ItsSudip,
shrouti1507,
sandeepdsvs,
krishna2020,
RanjeetMishra,
lvrach,
saikumarrs and
a team
as code owners
WalkthroughThe changes enhance the command-line argument handling in Changes
Possibly related PRs
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 (
|
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #1837 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 2 2
Lines 53 53
Branches 7 7
=========================================
Hits 53 53 ☔ View full report in Codecov by Sentry. |
Also add script to list json mapper destinations
koladilip
force-pushed
the
feat/dest-config-versioning
branch
from
a010320 to
00ce56d
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's limits when posting large numbers of comments.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (6)
scripts/listJsonMapperDestinations.js (2)
1-5: Consider modernizing to ES modulesWhile the current implementation works, consider updating to ES modules for better maintainability and alignment with modern JavaScript practices.
-/* eslint-disable no-console */ -const fs = require('fs'); -const path = require('path'); +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url));🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
48-48: Add proper error handling for script executionConsider adding proper exit codes for error scenarios to help with script automation.
-console.log(findJsonMapperDestinations().join('\n')); +try { + const destinations = findJsonMapperDestinations(); + if (destinations.length === 0) { + console.warn('No valid JSON mapper destinations found'); + process.exit(1); + } + console.log(destinations.join('\n')); + process.exit(0); +} catch (error) { + console.error('Failed to list destinations:', error); + process.exit(1); +}scripts/deployToDB.py (4)
Line range hint
16-44: Add validation for item_name argumentWhile the implementation is correct, consider adding validation to ensure the item exists before processing.
item_name = args.item_name or os.getenv("ITEM_NAME") + if item_name and not os.path.isdir(f"./{CONFIG_DIR}/destinations/{item_name}"): + print(f"Error: Item '{item_name}' not found in destinations directory") + sys.exit(1)
47-47: Consider reducing global variable usageGlobal variables can make the code harder to maintain and test. Consider refactoring to pass parameters directly to functions.
-CONTROL_PLANE_URL, USERNAME, PASSWORD, ITEM_NAME = get_command_line_arguments() +def main(): + control_plane_url, username, password, item_name = get_command_line_arguments() + run_updates(control_plane_url, username, password, item_name)
144-153: Enhance logging and validation in update_diff_dbThe function could benefit from better logging and validation:
- Use proper logging instead of print statements
- Add early validation for item existence
+import logging + def update_diff_db(selector, item_name=None): final_report = [] + logging.info(f"Starting update for selector: {selector}") if item_name: + if not os.path.isdir(f"./{CONFIG_DIR}/{selector}s/{item_name}"): + logging.error(f"Item '{item_name}' not found in {selector}s directory") + return final_report current_items = [item_name] + logging.info(f"Processing single item: {item_name}") else: current_items = os.listdir(f"./{CONFIG_DIR}/{selector}s") - - print(f"Current items: {current_items}") + logging.info(f"Processing {len(current_items)} items")
Line range hint
213-247: Improve logging structure in main executionConsider using a proper logging framework for better output control and formatting.
+import logging + if __name__ == "__main__": + logging.basicConfig( + level=logging.INFO, + format='%(asctime)s - %(levelname)s - %(message)s' + ) + - print("\n") - print("#" * 50) - print("Running Destination Definitions Updates") + logging.info("Running Destination Definitions Updates") dest_final_report = update_diff_db("destination", ITEM_NAME)
🛑 Comments failed to post (1)
scripts/listJsonMapperDestinations.js (1)
7-46: 🛠️ Refactor suggestion
Consider refactoring for better error handling and maintainability
The function could benefit from the following improvements:
- More specific error types for different failure scenarios
- Separation of concerns for filtering logic
- Memory efficiency for large directories
+const isValidDestination = (destination, config) => + !config?.disableJsonMapper && !config?.supportsVisualMapper; + +const readDestinationConfig = (destinationPath) => { + const content = fs.readFileSync(destinationPath, 'utf8'); + return JSON.parse(content); +}; + function findJsonMapperDestinations() { try { if (!fs.existsSync(destinationsDir)) { throw new Error(`Destinations directory not found: ${destinationsDir}`); } - return fs - .readdirSync(destinationsDir) - .map((destination) => { + const destinations = []; + for (const destination of fs.readdirSync(destinationsDir)) { try { const destinationsFilePath = path.join(destinationsDir, destination, 'db-config.json'); if (!fs.existsSync(destinationsFilePath)) { console.warn(`Skipping ${destination}: Missing configuration file`); - return null; + continue; } - const destinationsContent = fs.readFileSync(destinationsFilePath, 'utf8'); - const destinationDefinition = JSON.parse(destinationsContent); + const destinationDefinition = readDestinationConfig(destinationsFilePath); if (!destinationDefinition.name) { console.warn(`Skipping ${destination}: Missing name`); - return null; + continue; } - return { - name: destinationDefinition.name, - config: destinationDefinition.config, - }; + if (isValidDestination(destination, destinationDefinition.config)) { + destinations.push(destinationDefinition.name); + } } catch (err) { console.error(`Error processing ${destination}:`, err.message); - return null; } - }) - .filter(Boolean) - .filter( - (destination) => - !destination.config?.disableJsonMapper && !destination.config?.supportsVisualMapper, - ) - .map((destination) => destination.name); + } + return destinations; } catch (err) { console.error('Failed to process destinations:', err.message); return []; } }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.const isValidDestination = (destination, config) => !config?.disableJsonMapper && !config?.supportsVisualMapper; const readDestinationConfig = (destinationPath) => { const content = fs.readFileSync(destinationPath, 'utf8'); return JSON.parse(content); }; function findJsonMapperDestinations() { try { if (!fs.existsSync(destinationsDir)) { throw new Error(`Destinations directory not found: ${destinationsDir}`); } const destinations = []; for (const destination of fs.readdirSync(destinationsDir)) { try { const destinationsFilePath = path.join(destinationsDir, destination, 'db-config.json'); if (!fs.existsSync(destinationsFilePath)) { console.warn(`Skipping ${destination}: Missing configuration file`); continue; } const destinationDefinition = readDestinationConfig(destinationsFilePath); if (!destinationDefinition.name) { console.warn(`Skipping ${destination}: Missing name`); continue; } if (isValidDestination(destination, destinationDefinition.config)) { destinations.push(destinationDefinition.name); } } catch (err) { console.error(`Error processing ${destination}:`, err.message); } } return destinations; } catch (err) { console.error('Failed to process destinations:', err.message); return []; } }
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (5)
scripts/listJsonMapperDestinations.js (2)
1-5: Consider using ES modules instead of CommonJS requireThe code uses CommonJS require statements which are being phased out in favor of ES modules.
Consider updating to ES modules:
-const fs = require('fs'); -const path = require('path'); +import fs from 'fs'; +import path from 'path'; +import { fileURLToPath } from 'url'; + +const __dirname = path.dirname(fileURLToPath(import.meta.url));🧰 Tools
🪛 eslint
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
48-48: Consider handling script execution errorsThe script execution should handle potential promise rejection when the function is updated to async.
-console.log(findJsonMapperDestinations().join('\n')); +findJsonMapperDestinations() + .then(destinations => console.log(destinations.join('\n'))) + .catch(err => { + console.error('Script failed:', err); + process.exit(1); + });scripts/deployToDB.py (3)
16-18: Consider making item_name argument more descriptiveThe help text could be more descriptive about what types of items can be updated.
parser.add_argument( "item_name", nargs="?", - help="Specific item name to update.", default=None + help="Name of the specific destination, source, or wht-lib-project to update. If not provided, updates all items.", + default=None )Also applies to: 25-25
219-219: Consider adding progress indication for long-running operationsWhen updating multiple items, it would be helpful to show progress.
- dest_final_report = update_diff_db("destination", ITEM_NAME) + print(f"{'Updating specific destination: ' + ITEM_NAME if ITEM_NAME else 'Updating all destinations...'}") + dest_final_report = update_diff_db("destination", ITEM_NAME) - src_final_report = update_diff_db("source", ITEM_NAME) + print(f"{'Updating specific source: ' + ITEM_NAME if ITEM_NAME else 'Updating all sources...'}") + src_final_report = update_diff_db("source", ITEM_NAME) - wht_final_report = update_diff_db("wht-lib-project", ITEM_NAME) + print(f"{'Updating specific wht-lib-project: ' + ITEM_NAME if ITEM_NAME else 'Updating all wht-lib-projects...'}") + wht_final_report = update_diff_db("wht-lib-project", ITEM_NAME)Also applies to: 234-234, 249-249
Line range hint
146-194: Improve error handling for HTTP requestsThe function should handle HTTP request errors more gracefully and provide better error messages.
def update_diff_db(selector, item_name=None): final_report = [] + session = requests.Session() + session.auth = AUTH + session.headers.update(HEADER) + try: if item_name: item_path = f"./{CONFIG_DIR}/{selector}s/{item_name}" if not os.path.isdir(item_path): print(f"Error: Item '{item_name}' not found in {selector}s directory") return final_report current_items = [item_name] else: current_items = os.listdir(f"./{CONFIG_DIR}/{selector}s") print(f"Current items: {current_items}") for item in current_items: try: # check if item is a directory if not os.path.isdir(f"./{CONFIG_DIR}/{selector}s/{item}"): continue updated_data = get_file_content(item, selector) - persisted_data = get_config_definition( - CONTROL_PLANE_URL, selector, updated_data["name"] - ) + try: + persisted_data = get_config_definition( + CONTROL_PLANE_URL, selector, updated_data["name"] + ) + except requests.RequestException as e: + print(f"Error fetching configuration for {item}: {str(e)}") + continue if persisted_data.status_code == 200: # ... rest of the code ... + except json.JSONDecodeError as e: + print(f"Error parsing configuration for {item}: {str(e)}") + continue except Exception as e: - print(f"Error processing {item}: {str(e)}") + print(f"Unexpected error processing {item}: {str(e)}") continue + except Exception as e: + print(f"Fatal error in update_diff_db: {str(e)}") + raise return final_report
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
scripts/deployToDB.py(6 hunks)scripts/listJsonMapperDestinations.js(1 hunks)
🧰 Additional context used
🪛 eslint
scripts/listJsonMapperDestinations.js
[error] 2-2: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
[error] 3-3: Require statement not part of import statement.
(@typescript-eslint/no-var-requires)
🔇 Additional comments (1)
scripts/listJsonMapperDestinations.js (1)
38-40: Verify the filtering logic for JSON mapper destinations
The current logic excludes destinations where either disableJsonMapper is true OR supportsVisualMapper is true. Please verify if this is the intended behavior.
✅ Verification successful
Let me analyze the filtering logic based on the results. The code filters out destinations where either disableJsonMapper is true OR supportsVisualMapper is true. Looking at the data:
- Most destinations have
supportsVisualMapper=trueand varying values fordisableJsonMapper - Only a few destinations like BLOOMREACH have both flags set to false
- Some destinations like LYTICS_BULK_UPLOAD have
disableJsonMapper=falseandsupportsVisualMapper=null
Let's verify if this is documented behavior by checking the codebase for any related documentation or comments.
Based on the script name listJsonMapperDestinations.js and its implementation, I can now generate a final response about the filtering logic.
The filtering logic for JSON mapper destinations is correct
The function findJsonMapperDestinations() is specifically designed to list destinations that should use the JSON mapper, and the current filtering logic correctly excludes:
- Destinations that explicitly disable JSON mapper (
disableJsonMapper=true) - Destinations that support visual mapper (
supportsVisualMapper=true)
This makes sense because:
- If a destination has disabled JSON mapper, it shouldn't be included
- If a destination supports visual mapper, it uses that instead of JSON mapper
The data shows this is working as intended, allowing through only destinations like:
- BLOOMREACH (both flags false)
- LYTICS_BULK_UPLOAD (disableJsonMapper=false, supportsVisualMapper=null)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for destinations with disableJsonMapper or supportsVisualMapper flags
# to verify the filtering logic
fd -e json 'db-config.json' ./src/configurations/destinations -x jq -r '
select(.config.disableJsonMapper != null or .config.supportsVisualMapper != null) |
"\(.name): disableJsonMapper=\(.config.disableJsonMapper), supportsVisualMapper=\(.config.supportsVisualMapper)"
'
Length of output: 2558
Script:
#!/bin/bash
# Search for documentation or comments about JSON mapper and visual mapper
rg -i "jsonmapper|visualmapper" -g "!*.json" -g "!*.lock" -A 3 -B 3
Length of output: 1307
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
scripts/deployToDB.py (2)
11-13: Remove unnecessary empty linesThere are two consecutive empty lines after the
ALL_SELECTORSdefinition. One empty line is sufficient according to PEP 8.ALL_SELECTORS = ["destination", "source", "wht-lib-project"] -
237-251: Clean implementation but needs error handlingThe loop implementation is clean and improves code organization. However, consider adding error handling to catch and properly handle any exceptions that might occur during execution.
if __name__ == "__main__": + try: for selector in SELECTORS: print("\n") print("#" * 50) print("Running {} Definitions Updates".format(selector.capitalize())) final_report = update_diff_db(selector, ITEM_NAME) print("\n") print("#" * 50) print("{} Definition Update Report".format(selector.capitalize())) print(get_formatted_json(final_report)) print("\n") print("#" * 50) print("Stale {}s Report".format(selector.capitalize())) print(get_formatted_json(get_stale_data(selector, final_report))) + except Exception as e: + print(f"Error: Failed to process updates: {str(e)}") + sys.exit(1)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/deployToDB.py(3 hunks)
🔇 Additional comments (2)
scripts/deployToDB.py (2)
19-58: Well-structured argument handling with proper validation!
The implementation of the new command-line arguments is clean and follows best practices:
- Proper validation of selectors against
ALL_SELECTORS - Fallback to environment variables
- Clear error messages
- Sensible defaults
168-177:
Validate item existence before processing
The function should validate if the specified item exists before processing.
def update_diff_db(selector, item_name=None):
final_report = []
## data sets
if item_name:
+ item_path = f"./{CONFIG_DIR}/{selector}s/{item_name}"
+ if not os.path.isdir(item_path):
+ print(f"Error: Item '{item_name}' not found in {selector}s directory")
+ return final_report
current_items = [item_name]
else:
current_items = os.listdir(f"./{CONFIG_DIR}/{selector}s")
What are the changes introduced in this PR?
Right now the script runs for all destinations which takes a lot of time so updated the script to only run for a single destination.
Also add script to list JSON mapper destinations
What is the related Linear task?
Resolve INT-3012
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
Bug Fixes