Skip to content

Conversation

@robertbrignull
Copy link
Contributor

This PR makes the runner's parsing of the URL a bit more robust and should make it handle the case where GHES is using subdomain isolation.

I need to test that this actually works and I will do so before merging, but from my prelimanary experimentation it looks like it should do as the /api/v3 route on the main GHES domain still seems to work. So we don't need to be able to reconstruct the isolated subdomain.

The other changes to the URL parsing make it so if you don't pass a scheme then it'll assume it's https. Also makes the output URL more consistent.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.

@robertbrignull robertbrignull force-pushed the robertbrignull/subdomain branch from 0428734 to c4dc1b0 Compare September 28, 2020 17:40
Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

Looks like a nice improvement. 👍

@robertbrignull
Copy link
Contributor Author

I've verified manually that the /api/v3 path still seems to be available when subdomain isolation is turned on, so that means applications like ours can go from the main web URL to the API URL without having to know if subdomain isolation is turned on or not.

With the unit tests added in this PR I'm reasonable confident the URL parsing is working as expected. So this should all work fine.

@robertbrignull robertbrignull force-pushed the robertbrignull/subdomain branch from 2466f2a to 4b31f54 Compare October 5, 2020 15:38
@robertbrignull robertbrignull force-pushed the robertbrignull/subdomain branch from 4b31f54 to b185050 Compare October 5, 2020 15:45
@robertbrignull
Copy link
Contributor Author

This PR accidentally broke the break-out clause in getCodeQLBundleDownloadURL. By using GITHUB_DOTCOM_URL from parseGithubUrl we hopefully avoid that. Sorry for the false starts on this and force pushes. I've left in the trailing slash normalisation for non-dotcom URLs, but notably GITHUB_DOTCOM_URL does not have one as that constant matches the URL given to us by the hosted actions runners. The situation is now a little bit inconsistent but hopefully acceptable.

@chrisgavin assuming the tests now pass, it's still probably best if you check that last commit just to make I've not done anything silly.

Copy link
Contributor

@chrisgavin chrisgavin left a comment

Choose a reason for hiding this comment

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

Thanks! Yes, it looks right to me and the log output indicates it's following the short-circuit path so 👍 from me.

@robertbrignull robertbrignull merged commit 3630a78 into main Oct 5, 2020
@robertbrignull robertbrignull deleted the robertbrignull/subdomain branch October 5, 2020 16:53
@github-actions github-actions bot mentioned this pull request Oct 6, 2020
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