fix:[NEXT-291] renamed feature flag correctly #2353
Conversation
WalkthroughThe pull request introduces modifications to the Changes
Sequence DiagramsequenceDiagram
participant PolicyExecutor
participant DynamoDB
participant FeatureFlags
PolicyExecutor->>DynamoDB: Query TENANT_CONFIG_TABLE
DynamoDB-->>PolicyExecutor: Return tenant configuration
PolicyExecutor->>FeatureFlags: Check TENANT_FEATURE_FLAGS
FeatureFlags-->>PolicyExecutor: Return feature flag status
PolicyExecutor->>PolicyExecutor: Determine asset state service enablement
Poem
Tip CodeRabbit's docstrings feature is now available as part of our Early Access Program! Simply use the command 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: 0
🧹 Nitpick comments (1)
jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/executor/PolicyExecutor.java (1)
220-220: Enhance error logging with more contextThe error log should include the tenant ID to help with debugging issues.
- logger.error("Unable to fetch tenant config flags"); + logger.error("Unable to fetch tenant config flags for tenantId: {}", tenantId);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/common/PacmanSdkConstants.java(1 hunks)jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/executor/PolicyExecutor.java(1 hunks)
🔇 Additional comments (4)
jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/common/PacmanSdkConstants.java (2)
705-707: LGTM! Improved naming for feature flags
The renaming from API_FEATURE_FLAGS to TENANT_FEATURE_FLAGS better reflects the multi-tenant architecture. The addition of ENABLE_ASSET_STATE_SERVICE_FLAG_NAME provides clear context for asset state service configuration.
Let's verify the usage of these constants:
✅ Verification successful
Let me gather more context about the usage of these constants in PolicyExecutor.java to ensure the renaming is properly handled.
Verified: Feature flag renaming and usage is consistent
The changes are correctly implemented:
TENANT_FEATURE_FLAGSis properly used in PolicyExecutor.java for DynamoDB queriesENABLE_ASSET_STATE_SERVICE_FLAG_NAMEis correctly referenced to check the asset state service configuration- No instances of the old constant
API_FEATURE_FLAGSwere found in the codebase
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the old and new feature flag constants
echo "Searching for old and new feature flag usage..."
rg -l "API_FEATURE_FLAGS|TENANT_FEATURE_FLAGS"
# Search for asset state service configuration usage
echo "Searching for asset state service configuration..."
rg "ENABLE_ASSET_STATE_SERVICE_FLAG_NAME"
Length of output: 1160
Script:
#!/bin/bash
# Check the context of feature flag usage in PolicyExecutor.java
echo "Checking feature flag usage in PolicyExecutor.java..."
rg -A 5 -B 5 "TENANT_FEATURE_FLAGS|API_FEATURE_FLAGS" jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/executor/PolicyExecutor.java
Length of output: 1134
708-709: LGTM! Added tenant configuration constants
The new constants TENANT_CONFIG_TABLE and STATUS support tenant-specific configuration management. The naming is clear and follows the existing convention.
Let's verify the usage of these constants:
✅ Verification successful
Let me gather more context about the usage of these constants in the PolicyExecutor class to understand how they're being used.
Constants are properly used and integrated
The verification shows that:
TENANT_CONFIG_TABLEconstant is correctly used inPolicyExecutor.javafor DynamoDB table queries in the tenant configuration contextSTATUSconstant follows the existing pattern, as evidenced by similar declarations in other core components (admin, commons, aws-rules)- The implementation maintains consistency with the codebase's constant naming and usage patterns
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for references to the new tenant configuration constants
echo "Searching for tenant configuration usage..."
rg "TENANT_CONFIG_TABLE"
# Search for status constant usage
echo "Searching for status constant usage..."
rg "STATUS.*=.*\"status\""
Length of output: 1166
Script:
#!/bin/bash
# Check the usage context in PolicyExecutor
rg -A 5 -B 5 "TENANT_CONFIG_TABLE" jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/executor/PolicyExecutor.java
# Check if there are any other tenant-config related patterns
rg "tenant-config" --type java
Length of output: 1035
jobs/pacman-rule-engine-2.0/src/main/java/com/tmobile/pacman/executor/PolicyExecutor.java (2)
202-217: LGTM! Improved feature flag handling with constants
The changes properly use constants instead of magic strings and add appropriate null checks for the nested map structure.
225-225:
Fix logging parameters
The log message format is missing the parameters, which means the source and targetType values won't be logged.
- logger.info("sending asset state event for source: {} and target type: {}");
+ logger.info("sending asset state event for source: {} and target type: {}", source, targetType);Likely invalid or redundant comment.
|
Description
Please include a summary of the changes and the related issues. Please also include relevant motivation and context. List
any dependencies that are required for this change.
Problem
Solution
Fixes # (issue if any)
Type of change
Please delete options that are not relevant.
How Has This Been Tested?
Please describe the tests that you ran to verify your changes. Provide instructions so we can reproduce. Please also
list any relevant details for your test configuration
Checklist:
Other Information:
List any documentation updates that are needed for the Wiki
Summary by CodeRabbit
New Features
Bug Fixes
Improvements