KEMBAR78
Fix Helix URLs by RussKie · Pull Request #15672 · dotnet/arcade · GitHub
Skip to content

Conversation

@RussKie
Copy link
Contributor

@RussKie RussKie commented Mar 27, 2025

Fix Helix job's double slash which prevents users from navigating directly to the target.

Currently

image
image
image

Fix Helix job's double slash which prevents users from navigating directly to the target.
@RussKie RussKie requested review from mmitche and premun March 27, 2025 04:55
</PropertyGroup>
<Message Condition=" '$(HelixBaseUri)' == '' " Text="Sent Helix Job; see work items at https://helix.dot.net/api/jobs/$(HelixJobId)/workitems?api-version=2019-06-17$(_AccessTokenSuffix)" Importance="High" />
<Message Condition=" '$(HelixBaseUri)' != '' " Text="Sent Helix Job; see work items at $(HelixBaseUri)/api/jobs/$(HelixJobId)/workitems?api-version=2019-06-17$(_AccessTokenSuffix)" Importance="High" />
<Message Condition=" '$(HelixBaseUri)' != '' " Text="Sent Helix Job; see work items at $(HelixBaseUri)$(HelixBaseUri.EndsWith('/') ? '' : '/')api/jobs/$(HelixJobId)/workitems?api-version=2019-06-17$(_AccessTokenSuffix)" Importance="High" />
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't want to make any assumptions whether HelixBaseUri is "/"-terminated or not. Though, it is certainly an option.

@RussKie RussKie requested a review from Copilot March 27, 2025 04:55
Copy link

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.

Copilot wasn't able to review any files in this pull request.

Files not reviewed (1)
  • src/Microsoft.DotNet.Helix/Sdk/tools/Microsoft.DotNet.Helix.Sdk.MonoQueue.targets: Language not supported

Copy link
Member

@premun premun left a comment

Choose a reason for hiding this comment

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

Thanks for the change but I don't think it's implemented well.
Possibly You might need to add 2 message calls based on the EndsWith where one will include the slash and one won't

@akoeplinger akoeplinger requested a review from premun March 27, 2025 10:13
@premun premun merged commit a1332ad into main Mar 27, 2025
15 checks passed
@premun premun deleted the fix_double_helix_slash branch March 27, 2025 11:35
@RussKie
Copy link
Contributor Author

RussKie commented Mar 27, 2025

Thank you for your help!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants