fix: use url-join to properly support Travis Enterprise

[SimenB]

Closes https://github.com/semantic-release/condition-travis/pull/105

This feels like a better fix

[SimenB]

Don't merge this, it doesn't work (yet)...

[SimenB]

Never mind it works, I was fooled by travis caching.

I've had a successful deploy using this version 🙂

[pvdlg]

Regarding your mention of githubUrl and githubApiPathPrefix in semantic-release/condition-travis#105. We do that for Github as it's the way the API client works. In addition we are using the githubUrl to determine other things than the API URL (likes the issues, PR, commit, asset upload URL etc).

With Travis we don't do that and we use only the API URL.

Even if the doc mention the API is always in the /api context, I imagine the Travis Enterprise instance can be behind a reverse proxy and be exposed in a different path. Or it can be deployed in a different context (like /custom/api).

So I'm not sure adding the /api context might create some limitation in some specific cases. Does adding it brings any value? What's the difference for the user between setting http://custom.url/api vs http://custom.url in the environment variable / options?

Maybe it would be better to keep it as is and just mention in the documentation that the expected URL in enterprise is the complete API endpoint?

[SimenB]

We cannot keep it as is, as url.resolve('https://travis.example.com/api', 'auth/github') returns https://travis.example.com/auth/github' (first argument to resolve is stripped of its path).

[SimenB]

so either we need path explicitly passed in, or we need to join urls in some other way than url.resolve

[pvdlg]

We cannot keep it as is, as url.resolve('https://travis.example.com/api', 'auth/github') returns https://travis.example.com/auth/github' (first argument to resolve is stripped of its path).

Indeed. My mistake. This

return (await got(url.resolve(getTravisUrl(travisOpts), `builds/${buildId}`), {

should be

return (await got(url.resolve(getTravisUrl(travisOpts), `/builds/${buildId}`), {

There must be a / in front of builds/${buildId}.

Could you:

• revert your changes
• add the missing / (in two places I think)
• Modify the test Return job list with Travis Enterprise to add a prefix at the end of the URL
• Clarify in the doc that the URL expected is the full URL of the API

Thanks!

PS: If you can't I can take care of it tomorow

[SimenB]

url.resolve always strips the path, adding slashes doesn't help. 🙁

$ node -p "require('url').resolve('https://travis.example.com/api', '/builds/123')"
https://travis.example.com/builds/123
$ node -p "require('url').resolve('https://travis.example.com/api/', '/builds/123')"
https://travis.example.com/builds/123

[pvdlg]

Indeed, I misread the doc. Somehow, it's recommended in many different places to use resolve to concat URL, but it seems to really not be a good idea.

What do you think about the following?

// get-travis-url.js

module.exports = ({enterprise, pro} = {}) =>
  (enterprise || (pro ? 'https://api.travis-ci.com' : 'https://api.travis-ci.org')).replace(/[^/]$/, '$&/')

// get-jobs and and get-token.js

await got(`${getTravisUrl(travisOpts)}builds/${buildId}`), {...}

[SimenB]

That feels convoluted, what about just adding https://www.npmjs.com/package/url-join? Seems to solve the problem pretty good.

Should be drop in replacement for url.resolve

[pvdlg]

Ok. It's crazy there is no standard Node API do that...It's a really common thing to do...

[SimenB]

Pushed 🙂

[SimenB]

Pushed a commit reverting the changes to the tests.

My thinking was that having the tests test what did not work before was a good idea, but we still keep 100% coverage without changing tests, so 🤷‍♂️

EDIT: Oh conflicts. I'll rebase 🙂 Squashed while I was at it

[pvdlg]

My thinking was that having the tests test what did not work before was a good idea, but we still keep 100% coverage without changing tests, so 🤷‍♂️

Yes we should do that by testing with /api in test/get-jobs.test.js and test/get-token.test.js.

See comment and comment

[SimenB]

Perfect, I misunderstood the API of nock, I though nock(url) could just be the host, not path. Re-added /api now, and can verify that with url.resolve the two tests fail. Thanks!

[SimenB]

Yay! I'll test it out tomorrow at work and report back :)