Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix endpoint detection logic, broken on FetchError #608

Merged
merged 2 commits into from
Jul 4, 2021

Conversation

0x2b3bfa0
Copy link
Member

@0x2b3bfa0 0x2b3bfa0 commented Jun 25, 2021

Detecting errors other than the vanilla Error isn't especially easy nor reliable, or so it seems. Given that we only return errors and strings, it's easier to consider as an error everything that isn't a string.

FetchError subclasses Error, but detecting it would be a pain
@0x2b3bfa0 0x2b3bfa0 added bug Something isn't working technical-debt Refactoring, linting & tidying ci-gitlab labels Jun 25, 2021
@0x2b3bfa0 0x2b3bfa0 self-assigned this Jun 25, 2021
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 25, 2021 16:18 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 25, 2021 16:19 Inactive
@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Jun 25, 2021

Spent 30+ minutes figuring out why this user was querying a project with a broken full address beginning with ttps instead of the usual user/project format.

It turns out that the projectPath function wasn't checking if repo.startsWith(repoBase) before running repo.replace(repoBase, '').substr(1) and it just worsened the issue by removing the first character without removing the expected prefix.

Are my deffensive programming instincts worth the time? At least in certain cases, these extra checks could be within reason.

@DavidGOrtega
Copy link
Contributor

Not sure if this is solving the real issue. And of course nothing to be with the original case. The problem is that in CML.js we assume always https.

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Jun 28, 2021

Not sure if this is solving the real issue.

This pull request just fixes a bug where every connection error gets masked under a barely meaningful TypeError: Only absolute URLs are supported error. Not sure if that's the real issue you mention, but at least it helps us to give unerring replies to users with real issues that are reporting the mentioned unhelpful error.

And of course nothing to be with the original case.

If you mean this Discord conversation, it would have been impossible to provide an accurate solution without the code from this pull request. 🤔

The problem is that in CML.js we assume always https.

That's a good default, but some self-hosted users might still be relying on bare HTTP. It looks like you've identified another potential issue.

@DavidGOrtega
Copy link
Contributor

DavidGOrtega commented Jun 28, 2021

That's a good default, but some self-hosted users might still be relying on bare HTTP. It looks like you've identified another potential issue.

I think thats the real issue of your user.

All this pain could be solved checking http vs https since http wont even work either in any circumstance

@0x2b3bfa0
Copy link
Member Author

0x2b3bfa0 commented Jun 28, 2021

I think thats the real issue of your user.

It looks like "my" user had a slightly more creative issue, where a plain HTTP server was listening on port 443 instead of 80.

All this pain could be solved checking http vs https since http wont even work either in any circumstance

How come? Moved to #616

@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 17:56 Inactive
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 18:38 Inactive
@0x2b3bfa0 0x2b3bfa0 force-pushed the improve-error-checking-gitlab-api branch from c9b0060 to fb9e082 Compare June 30, 2021 20:05
@0x2b3bfa0 0x2b3bfa0 temporarily deployed to internal June 30, 2021 20:05 Inactive
@DavidGOrtega DavidGOrtega merged commit f5f96d8 into master Jul 4, 2021
@DavidGOrtega DavidGOrtega deleted the improve-error-checking-gitlab-api branch July 4, 2021 21:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working ci-gitlab technical-debt Refactoring, linting & tidying
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants