Skip to content

Sync eng/common directory with azure-sdk-tools for PR 10532#45329

Merged
hallipr merged 3 commits intomainfrom
sync-eng/common-users/pahallis/resources-10532
May 14, 2025
Merged

Sync eng/common directory with azure-sdk-tools for PR 10532#45329
hallipr merged 3 commits intomainfrom
sync-eng/common-users/pahallis/resources-10532

Conversation

@azure-sdk
Copy link
Copy Markdown
Collaborator

Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#10532 See eng/common workflow

Copilot AI review requested due to automatic review settings May 14, 2025 18:31
@azure-sdk azure-sdk requested a review from a team as a code owner May 14, 2025 18:31
@azure-sdk azure-sdk requested a review from hallipr May 14, 2025 18:31
@azure-sdk azure-sdk added EngSys This issue is impacting the engineering system. Central-EngSys This issue is owned by the Engineering System team. labels May 14, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This pull request synchronizes the eng/common directory with azure-sdk-tools and updates several test resource scripts for consistency with the latest workflow. Key changes include updating default parameter values, adding conditional checks to safeguard processing for empty inputs, and modifying parameter attributes to standardize handling of the ServiceDirectory value.

Reviewed Changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated no comments.

File Description
eng/common/TestResources/deploy-test-resources.yml Updated default ServiceDirectory value from "not-set" to an empty string.
eng/common/TestResources/SubConfig-Helpers.ps1 Added a guard clause for empty serviceName and adjusted resource group naming logic based on serviceDirectoryName.
eng/common/TestResources/Remove-TestResources.ps1 Removed the early exit check for empty ServiceDirectory in CI and updated the path resolution logic.
eng/common/TestResources/New-TestResources.ps1 Made ServiceDirectory optional and updated resource path and tagging logic accordingly.
Comments suppressed due to low confidence (4)

eng/common/TestResources/Remove-TestResources.ps1:159

  • The removal of the early exit for an empty ServiceDirectory in CI mode may lead to unintended processing downstream. Consider reintroducing a conditional check to handle cases when ServiceDirectory is not provided.
if (!$ServiceDirectory) {

eng/common/TestResources/deploy-test-resources.yml:2

  • Changing the default value for ServiceDirectory from 'not-set' to an empty string may affect consumers expecting a placeholder. Please verify that downstream workflows handle empty values correctly.
ServiceDirectory: ''

eng/common/TestResources/SubConfig-Helpers.ps1:38

  • Using $serviceDirectoryName.Length when applying a substring on $serviceName could be inconsistent. Confirm that this logic is intended and that the correct variable is used for determining the substring length.
if ($serviceDirectoryName) {

eng/common/TestResources/New-TestResources.ps1:21

  • Changing the ServiceDirectory parameter from mandatory to optional alters the expected contract. Ensure that downstream logic correctly handles situations when ServiceDirectory is missing.
[Parameter(Position = 0)]

@hallipr
Copy link
Copy Markdown
Member

hallipr commented May 14, 2025

/check-enforcer override

The changed files are not invoked as part of the recorded test run. The single failing test is unrelated to the files changed.

@hallipr hallipr merged commit b653ec4 into main May 14, 2025
47 of 49 checks passed
@hallipr hallipr deleted the sync-eng/common-users/pahallis/resources-10532 branch May 14, 2025 19:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Central-EngSys This issue is owned by the Engineering System team. EngSys This issue is impacting the engineering system.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants