Reviewstack seems to always use "api.HOSTNAME" despite this not working for GitHub Enterprise

[Icantjuddle]

Main github.com has its rest apis at api.github.com whereas GHE has them at example.com/api/v3. This fact is used in a few places to correctly deduce the format but not everywhere. Github has a note about which URL to use here:

Correct: https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/gitHubCredentials.ts#L253-L273

Not working: https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/GraphQLGitHubClient.ts#L173

https://github.com/facebook/sapling/blob/2f15e589a5b2073546b223656c920fa837fe0bb6/addons/reviewstack/src/github/GraphQLGitHubClient.ts#L220

[evangrayk]

Thanks for reporting this. I tried this out on a GHE instance with ${hostname}/api for these fetches you pointed out and I ended up getting a CORS error, while api.${hostname} worked fine (/api/v3 also didn't work for me). So I guess this somehow depends on your particular GHE instance.

Not sure what we should do here if one format works for some instances but not for others and vice-versa.

cc @bolinfest in case you have ideas

[bolinfest]

@Icantjuddle out of curiosity, have you tried building ReviewStack locally and making the necessary change to see if it works with your GHE instance?

@evangrayk I'll try to repro. I'm surprised ${hostname}/api would yield a CORS error since we are using https://${hostname}/api/graphql for GraphQL (as opposed to REST) requests, which seems to work fine? To clarify, instead of:

const url = `https://api.${this.hostname}/repos/${encodeURIComponent(

you tried:

const url = `https://${this.hostname}/api/v3/repos/${encodeURIComponent(

[Icantjuddle]

@bolinfest

out of curiosity, have you tried building ReviewStack locally

Yes I have been, was hoping to validate this more fully before filing the issue; however, unfortunately I'm not very familiar with the javascript ecosystem and have been struggling to build it correctly with changes. Here is what I'm doing, and what I've tried, maybe you can see what I'm doing wrong.

1. I make the appropriate changes in addons/reviewstack
2. I run yarn codegen (not sure this is needed)
3. Then go to addons/reviewstack.dev to yarn build
4. Then I serve the build folder with twisted with a self-signed cert (I'm presuming I need to serve over TLS so CORS is happy, since our GHE is only served over TLS).

The problem I've noticed is that the requests are still going to the api.${hostname} even after I've changed those strings (verified in the browser console). So I think somehow the build in addons/reviewstack.dev is not using the changes I've made to addons/reivewstack. I noticed in addons/reviewstack.dev/package.json that it specifies a version of reviewstack so presumably it is using that over the one in the repo. I googled around and found that yarn link should be able to help here so I did the following with no result:

1. Ran yarn link in addons/reviewstack
2. Ran yarn link reviewstack in addons/reviewstack.dev
3. Deleted the build folder in addons/reviewstack.dev and tried the above process again.

This however did not help and it was still using the old request url: api.${hostname}.

If you know a way I can get the build picked up correctly please LMK and I would be happy to test.

---

I was able to verify my suggestion (not the code change) in a different way though. I installed a browser extension requestly and configured it to rewrite URLS matching api.${hostname} to ${hostname}/api/v3/ and the CORS errors go away and I see the main body of the PR view rendered. However, I still do not see my file diff from the PR rendered in that view, but I'm presuming that is a separate issue.

[Icantjuddle]

@bolinfest any thoughts on ^?

[Icantjuddle]

@bolinfest just checking in, any thoughts?

[Icantjuddle]

@bolinfest @evangrayk Just checking in again, would be happy to put in a PR if you can help me with the local build/test process per the msg above.