TECH_DEBT: Normalize branch names in pipelines#1485
Open
seanmcilvenna wants to merge 1 commit intodevfrom
Open
TECH_DEBT: Normalize branch names in pipelines#1485seanmcilvenna wants to merge 1 commit intodevfrom
seanmcilvenna wants to merge 1 commit intodevfrom
Conversation
Contributor
📝 WalkthroughWalkthroughStandardizes image tagging across 17 Azure Pipeline deployment files by introducing a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
Tip Try Coding Plans. Let us write the prompt for your AI agent so you can ship faster (with fewer bugs). Comment |
… hyphens for consistency in tags and image names. Update `list-deploy-changes.py` to include per-service version comparisons and warnings for non-incremented versions.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (4)
Azure_Pipelines/azure-pipelines.terminology.cd.yaml (1)
95-102: Duplicate MyTag assignment — consider consolidating.Both PowerShell blocks (lines 66-77 and 91-102) set
MyTagwith identical logic. The second block overwrites the first. This is pre-existing, but worth consolidating in a future cleanup to avoid confusion.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Azure_Pipelines/azure-pipelines.terminology.cd.yaml` around lines 95 - 102, The MyTag variable is being set twice with identical logic (based on Build.SourceBranch, CLEANED_BRANCH_NAME and GIT_COMMIT), causing the second assignment to overwrite the first; remove the duplicate block and keep a single consolidated PowerShell block that computes $myTag1 using the conditional on "$(Build.SourceBranch)" -like "*release/*" and then sets the pipeline variable via Write-Host "##vso[task.setvariable variable=MyTag]$myTag1" and logs "Set MyTag to: $myTag1" so the logic for MyTag (references: MyTag, $myTag1, CLEANED_BRANCH_NAME, GIT_COMMIT, Build.SourceBranch) exists only once.Scripts/list-deploy-changes.py (3)
74-117: Consider narrowing exception handlers for better debuggability.The broad
except Exceptionblocks (lines 89, 110) will silently swallow unexpected errors during XML parsing. While the fallback to regex is a good safety net, catching more specific exceptions (e.g.,ET.ParseError,AttributeError) would help surface unexpected issues during development.Additionally, static analysis flags that
xml.etree.ElementTreecan be vulnerable to XML attacks (S314). Since the content comes from git refs within the repository, the risk is low, but consider usingdefusedxmlif this script ever processes external input.🔧 Optional: Narrow exception handling
try: root = ET.fromstring(content) # In .csproj, Version is usually in a PropertyGroup v = root.find(".//Version") if v is not None and v.text: return v.text.strip() - except Exception: + except (ET.ParseError, AttributeError): # Fallback to regex match = re.search(r"<Version>(.*?)</Version>", content) if match: return match.group(1).strip()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/list-deploy-changes.py` around lines 74 - 117, The get_file_version function currently uses broad "except Exception" handlers around XML parsing; narrow these to catch specific parsing errors (e.g., xml.etree.ElementTree.ParseError) and any attribute-related errors (e.g., AttributeError) instead of Exception for both the .csproj and pom.xml branches, so genuine unexpected errors aren't silently swallowed; optionally, if you want safer XML parsing for external inputs, switch to defusedxml.ElementTree (or parse via defusedxml) and update the exception types accordingly (reference the function get_file_version and the try/except blocks surrounding ET.fromstring and the subsequent XML find logic).
232-232: Use proper type annotation instead of comment.The inline comment
# Dict[str, List[str]]should be a type annotation for consistency with the type hints elsewhere in this file.🔧 Suggested fix
- version_to_services = {} # Dict[str, List[str]] + version_to_services: dict[str, list[str]] = {}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/list-deploy-changes.py` at line 232, The variable version_to_services is using a comment for its type; replace the inline comment with a proper type annotation (e.g., version_to_services: Dict[str, List[str]] = {} or, if the file uses modern annotations, version_to_services: dict[str, list[str]] = {}) and ensure the corresponding typing imports are present (add from typing import Dict, List if using Dict/List form) so the annotation matches the style used elsewhere in the file.
119-124: Add more specific type hint for clarity.The return type
Tupleis generic. Consider specifying the content type for better IDE support and documentation.🔧 Suggested type hint improvement
-def version_to_tuple(v: str) -> Tuple: +def version_to_tuple(v: str) -> Tuple[int, ...]: """Convert version string to a tuple of integers for comparison.""" try: return tuple(int(p) for p in re.findall(r'\d+', v)) - except Exception: + except (ValueError, TypeError): return (v,)Note: If keeping the string fallback
(v,), the return type would need to beTuple[int, ...] | Tuple[str]ortupleto be accurate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@Scripts/list-deploy-changes.py` around lines 119 - 124, The return type of version_to_tuple is currently the generic Tuple; update its type hint to reflect that it returns either a tuple of ints or a single-string tuple (e.g., change to Tuple[int, ...] | Tuple[str] or the equivalent tuple[int, ...] | tuple[str] depending on your Python typing target) and update the function signature of version_to_tuple(v: str) accordingly so IDEs and linters can infer the correct element types when it returns the parsed int tuple or the fallback (v,) string tuple.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@Azure_Pipelines/azure-pipelines.terminology.cd.yaml`:
- Around line 95-102: The MyTag variable is being set twice with identical logic
(based on Build.SourceBranch, CLEANED_BRANCH_NAME and GIT_COMMIT), causing the
second assignment to overwrite the first; remove the duplicate block and keep a
single consolidated PowerShell block that computes $myTag1 using the conditional
on "$(Build.SourceBranch)" -like "*release/*" and then sets the pipeline
variable via Write-Host "##vso[task.setvariable variable=MyTag]$myTag1" and logs
"Set MyTag to: $myTag1" so the logic for MyTag (references: MyTag, $myTag1,
CLEANED_BRANCH_NAME, GIT_COMMIT, Build.SourceBranch) exists only once.
In `@Scripts/list-deploy-changes.py`:
- Around line 74-117: The get_file_version function currently uses broad "except
Exception" handlers around XML parsing; narrow these to catch specific parsing
errors (e.g., xml.etree.ElementTree.ParseError) and any attribute-related errors
(e.g., AttributeError) instead of Exception for both the .csproj and pom.xml
branches, so genuine unexpected errors aren't silently swallowed; optionally, if
you want safer XML parsing for external inputs, switch to defusedxml.ElementTree
(or parse via defusedxml) and update the exception types accordingly (reference
the function get_file_version and the try/except blocks surrounding
ET.fromstring and the subsequent XML find logic).
- Line 232: The variable version_to_services is using a comment for its type;
replace the inline comment with a proper type annotation (e.g.,
version_to_services: Dict[str, List[str]] = {} or, if the file uses modern
annotations, version_to_services: dict[str, list[str]] = {}) and ensure the
corresponding typing imports are present (add from typing import Dict, List if
using Dict/List form) so the annotation matches the style used elsewhere in the
file.
- Around line 119-124: The return type of version_to_tuple is currently the
generic Tuple; update its type hint to reflect that it returns either a tuple of
ints or a single-string tuple (e.g., change to Tuple[int, ...] | Tuple[str] or
the equivalent tuple[int, ...] | tuple[str] depending on your Python typing
target) and update the function signature of version_to_tuple(v: str)
accordingly so IDEs and linters can infer the correct element types when it
returns the parsed int tuple or the fallback (v,) string tuple.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 38fbd908-4d4d-4616-a66a-fd8fd87d202d
📒 Files selected for processing (19)
Azure_Pipelines/_deploy_all_services.ymlAzure_Pipelines/azure-pipelines.account.cd.yamlAzure_Pipelines/azure-pipelines.admin-ui.cd.yamlAzure_Pipelines/azure-pipelines.admin.cd.yamlAzure_Pipelines/azure-pipelines.audit.cd.yamlAzure_Pipelines/azure-pipelines.census.cd.yamlAzure_Pipelines/azure-pipelines.dataacquisition-worker.cd.yamlAzure_Pipelines/azure-pipelines.dataacquisition.cd.yamlAzure_Pipelines/azure-pipelines.ec.cd.yamlAzure_Pipelines/azure-pipelines.measureeval.cd.yamlAzure_Pipelines/azure-pipelines.normalization.cd.yamlAzure_Pipelines/azure-pipelines.querydispatch.cd.yamlAzure_Pipelines/azure-pipelines.report.cd.yamlAzure_Pipelines/azure-pipelines.submission.cd.yamlAzure_Pipelines/azure-pipelines.tenant.cd.yamlAzure_Pipelines/azure-pipelines.terminology.cd.yamlAzure_Pipelines/azure-pipelines.validation.cd.yamlAzure_Pipelines/deploy_tags_all.ymlScripts/list-deploy-changes.py
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
🛠️ Description of Changes
Normalize branch names in pipelines by replacing forward slashes with hyphens for consistency in tags and image names. Update
list-deploy-changes.pyto include per-service version comparisons and warnings for non-incremented versions.🧪 Testing Performed
Please describe the testing that was performed on the changes included in this PR.
🧑🔬 Unit Testing
📓 Documentation Updated
Please update any relevant sections in the project documentation that were impacted by the changes in the PR.
Summary by CodeRabbit
Release Notes
New Features
Chores