Skip to content

Make the default github.com path consistent with other paths#401

Closed
aeisenberg wants to merge 1 commit intogithub:mainfrom
aeisenberg:aeisenber/canonicalize-path
Closed

Make the default github.com path consistent with other paths#401
aeisenberg wants to merge 1 commit intogithub:mainfrom
aeisenberg:aeisenber/canonicalize-path

Conversation

@aeisenberg
Copy link
Contributor

@aeisenberg aeisenberg commented Feb 23, 2021

On util.ts:223, there is the following comment:

// Normalise path to having a trailing slash for consistency

However, I notice that the path defined by GITHUB_DOTCOM_URL
does not have a trailing slash. This PR fixes it.

I'm not 100% sure this is valid, so feel free to close if it doesn't make sense.

Though, if we don't go with this PR, then we should probably remove the requirement for a trailing / for other URIs.

Merge / deployment checklist

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

On util.ts:223, there is the following comment:

```
// Normalise path to having a trailing slash for consistency
```

However, I notice that the path defined by `GITHUB_DOTCOM_URL`
does not have a trailing slash. This PR fixes it.
@robertbrignull
Copy link
Contributor

robertbrignull commented Feb 24, 2021

One thing to note is that constant currently has to match the GITHUB_SERVER_URL env var on actions on github.com. Look in actions-util.ts where we use this. But that check is a bit brittle and would probably be better off parsing the URL and checking the hostname instead of check for textual equality. If you change that usage then I think this change would be fine.

Something else I also spotted is in codeql.ts we do if (codeqlURL.startsWith(`${apiDetails.url}/`) which I think it going to be wrong if the API URL always has a trailing slash. Would be good to look into that.

@aeisenberg aeisenberg closed this May 3, 2021
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.

2 participants