feat:[PEA-31] Tagging rule changes and scripts for Tagging summary#2393
Conversation
WalkthroughAdds dual-mode mandatory tag validation to EC2 TaggingRule: new rule parameters for case-sensitive and case-insensitive tag lists, a case-aware PacmanUtils helper to compute missing tags, fallback to configured tags when parameters are absent, and a state null guard for EC2 resources. ChangesEC2 Instance Tagging Rule with Case-Sensitive/Insensitive Tag Support
Sequence Diagram(s)sequenceDiagram
participant TaggingRule
participant Config
participant PacmanUtils
Note over TaggingRule: Evaluate EC2 resource
TaggingRule->>Config: read TAGGING_MANDATORY_TAGS (fallback)
TaggingRule->>PacmanUtils: getMissingTagsfromResourceAttributeWithCase(caseSensitiveList, caseInsensitiveList, attributes)
PacmanUtils-->>TaggingRule: Set of missing tags
Note over TaggingRule: build annotation payload using combined mandatory tags
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.42.2)jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.javaWarning Review ran into problems🔥 ProblemsStopped waiting for pipeline failures after 30000ms. One of your pipelines takes longer than our 30000ms fetch window to run, so review may not consider pipeline-failure results for inline comments if any failures occurred after the fetch window. Increase the timeout if you want to wait longer or run a Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/ec2/TaggingRule.java`:
- Around line 83-112: Ensure shared required inputs (tagsSplitter, targetType
and any other required config) are validated before computing hasCustomTags and
entering the custom-tag branch: check that tagsSplitter and targetType are
non-null/non-empty and call PacmanUtils.doesAllHaveValue or throw
InvalidInputException early. Also make state checks null-safe: verify
resourceAttributes is non-null and that
resourceAttributes.get(PacmanRuleConstants.STATE_NAME) != null before calling
equalsIgnoreCase (use a local String state = resourceAttributes.get(...); if
(state != null && state.equalsIgnoreCase(...))). Update references in this block
(hasCustomTags, caseSensitiveTagsFromParam, caseInsensitiveTagsFromParam,
tagsSplitter, targetType, resourceAttributes, PacmanRuleConstants.STATE_NAME)
accordingly.
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.java`:
- Around line 153-163: When normalizing tag keys in PacmanUtils, the current
loop that builds lowerCaseAttributes overwrites case-variant entries and can
replace a populated value with a blank one; change the population logic in the
loop that constructs lowerCaseAttributes so it only replaces an existing entry
when the existing value is null or blank and the new value is non-blank (i.e.,
compute lowerKey = entry.getKey().toLowerCase(), existing =
lowerCaseAttributes.get(lowerKey); if existing==null or
(existing.trim().isEmpty() && !entry.getValue().trim().isEmpty()) then put the
new value). This uses the existing lowerCaseAttributes map, the entry from
attributes, and getTagKey/caseInsensitiveTags to preserve non-blank values when
normalizing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: b78f7b22-1ab9-4bf4-945f-4c175aabe32c
📒 Files selected for processing (4)
installer/resources/pacbot_app/files/DB_Policy.sqljobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/ec2/TaggingRule.javajobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.javajobs/pacman-awsrules/src/main/java/com/tmobile/cloud/constants/PacmanRuleConstants.java
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.java`:
- Line 156: The toLowerCase() call used to normalize tag keys (e.g., where
lowerKey is assigned via entry.getKey().toLowerCase()) is locale-sensitive;
change it to entry.getKey().toLowerCase(Locale.ROOT) (and any other
toLowerCase() occurrences in the same method/area such as the one around line
163) to ensure locale-safe normalization, and add the import java.util.Locale if
it's not already present.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 010747ac-0e2b-419e-b5ab-c56618e89a3a
📒 Files selected for processing (1)
jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.java
| Map<String, String> lowerCaseAttributes = new HashMap<>(); | ||
|
|
||
| for (Map.Entry<String, String> entry : attributes.entrySet()) { | ||
| String lowerKey = entry.getKey().toLowerCase(); |
There was a problem hiding this comment.
Specify Locale.ROOT in toLowerCase() calls to prevent locale-specific bugs.
The case normalization uses toLowerCase() without specifying a locale. In Turkish and some other locales, this produces incorrect results (e.g., "I".toLowerCase() yields "ı" instead of "i"), which will cause tag keys to be mismatched and tags incorrectly reported as missing.
The past review comment on lines 153–165 explicitly suggested toLowerCase(Locale.ROOT) to avoid this issue, but that change was not applied.
🔧 Apply the locale-safe normalization
for (Map.Entry<String, String> entry : attributes.entrySet()) {
- String lowerKey = entry.getKey().toLowerCase();
+ String lowerKey = entry.getKey().toLowerCase(Locale.ROOT);
String existingValue = lowerCaseAttributes.get(lowerKey);
if (existingValue == null || existingValue.trim().isEmpty()) {
lowerCaseAttributes.put(lowerKey, entry.getValue());
}
}
for (String tag : caseInsensitiveTags) {
- String expectedKey = getTagKey(tag).toLowerCase();
+ String expectedKey = getTagKey(tag).toLowerCase(Locale.ROOT);
String value = lowerCaseAttributes.get(expectedKey);Also applies to: 163-163
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@jobs/pacman-awsrules/src/main/java/com/tmobile/cloud/awsrules/utils/PacmanUtils.java`
at line 156, The toLowerCase() call used to normalize tag keys (e.g., where
lowerKey is assigned via entry.getKey().toLowerCase()) is locale-sensitive;
change it to entry.getKey().toLowerCase(Locale.ROOT) (and any other
toLowerCase() occurrences in the same method/area such as the one around line
163) to ensure locale-safe normalization, and add the import java.util.Locale if
it's not already present.
2 participants
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