-
Notifications
You must be signed in to change notification settings - Fork 228
Fix npm package URL transformation to handle versioned URLs and edge cases correctly #13307
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
eng/common/scripts/Verify-Links.ps1
Outdated
| # Handle non-versioned URLs: https://www.npmjs.com/package/@azure/ai-agents -> https://registry.npmjs.org/@azure/ai-agents | ||
| # The regex captures the package name (which may contain a slash for scoped packages) and optionally the version | ||
| $urlString = $linkUri.ToString() | ||
| if ($urlString -match '^https?://(?:www\.)?npmjs\.com/package/([^/]+(?:/[^/]+)?)/v/(.+)$') { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| if ($urlString -match '^https?://(?:www\.)?npmjs\.com/package/([^/]+(?:/[^/]+)?)/v/(.+)$') { | |
| if ($urlString -match '^https?://(?:www\.)?npmjs\.com/package/(.+)/v/(.+)$') { |
@copilot you can simplify this regex with the suggestion.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Simplified the regex pattern as suggested. Commit a36dad8 changes ([^/]+(?:/[^/]+)?) to (.+) in both the main code and test helper function. All tests pass with the simplified pattern.
Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
|
/azp run azure-sdk-tools - sync - eng-common |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this 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 PR fixes the npm package URL transformation logic in the link verification script to correctly handle versioned npm package URLs. The implementation removes the literal "/v/" path segment from versioned URLs while preserving the version number, and maintains non-versioned URLs unchanged.
Key Changes:
- Updated regex-based URL transformation in
ProcessNpmLinkfunction to handle both versioned and non-versioned npm package URLs - Added comprehensive test suite with 8 test cases covering scoped packages, unscoped packages, URLs with/without "www" prefix, and version variations
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| eng/common/scripts/Verify-Links.ps1 | Replaced single-line regex transformation with conditional logic to handle versioned URLs (removing "/v/" segment) and non-versioned URLs separately |
| eng/common-tests/Verify-Links-ProcessNpmLink.Tests.ps1 | Added new test file with 8 test cases validating URL transformations for various npm package URL formats |
|
The following pipelines have been queued for testing: |
Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
|
/azp run azure-sdk-tools - sync - eng-common |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
/azp run azure-sdk-tools - sync - eng-common |
|
Azure Pipelines successfully started running 1 pipeline(s). |
|
The following pipelines have been queued for testing: |
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#13307 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
Sync eng/common directory with azure-sdk-tools for PR Azure/azure-sdk-tools#13307 See [eng/common workflow](https://github.com/Azure/azure-sdk-tools/blob/main/eng/common/README.md#workflow) --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
…cases correctly (#13307) * Initial plan * Fix npm URL replacement logic to handle versioned and non-versioned URLs Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com> * Refactor tests to reduce duplication with helper function Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com> * Simplify regex pattern as suggested by weshaggard Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com> * Fix regex to handle query params and fragments, add edge case tests Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com> * Improve test helper documentation per feedback Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com> --------- Co-authored-by: copilot-swe-agent[bot] <198982749+Copilot@users.noreply.github.com> Co-authored-by: weshaggard <9010698+weshaggard@users.noreply.github.com>
https://www.npmjs.com/package/@azure/ai-agents/v/1.1.0→https://registry.npmjs.org/@azure/ai-agents/1.1.0https://www.npmjs.com/package/@azure/ai-agents→https://registry.npmjs.org/@azure/ai-agentsSummary
Successfully updated the npm URL replacement logic in
eng/common/scripts/Verify-Links.ps1to correctly handle both versioned and non-versioned npmjs package URLs.Key Changes
([^/]+(?:/[^/]+)?)to([^?#]+)based on review feedbackAll 12 tests pass successfully. The implementation now correctly strips query parameters and fragments from URLs before transformation, ensuring accurate conversion to registry.npmjs.org URLs.
Original prompt
This pull request was created as a result of the following prompt from Copilot chat.
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.