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(utils): accept DSN URLs with empty password #5902

Merged
merged 4 commits into from Oct 7, 2022

Conversation

JonasKruckenberg
Copy link
Contributor

Before submitting a pull request, please take a look at our
Contributing guidelines and verify:

  • If you've added code that should be tested, please add tests.
  • Ensure your code lints and the test suite passes (yarn lint) & (yarn test).

@AbhiPrasad AbhiPrasad changed the title fix: accept DSN URLs with empty password fix(utils): accept DSN URLs with empty password Oct 6, 2022
@AbhiPrasad
Copy link
Member

Thanks for opening a PR!

packages/utils/src/dsn.ts Fixed Show fixed Hide fixed
@AbhiPrasad AbhiPrasad requested review from a team, Lms24, AbhiPrasad and lobsterkatie and removed request for a team October 6, 2022 11:56
Copy link
Member

@Lms24 Lms24 left a comment

Choose a reason for hiding this comment

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

Thanks for opening up this PR. I have a suggestion (see below) but another pair of eyes on this probably won't hurt

packages/utils/src/dsn.ts Outdated Show resolved Hide resolved
Co-authored-by: Lukas Stracke <lukas.stracke@sentry.io>
@Lms24
Copy link
Member

Lms24 commented Oct 6, 2022

Seems like prettier isn't yet happy. @JonasKruckenberg, would you mind running yarn fix to fix the linter error(s)?
(The other two failed jobs are probably just test flakes)

@JonasKruckenberg
Copy link
Contributor Author

Seems like prettier isn't yet happy. @JonasKruckenberg, would you mind running yarn fix to fix the linter error(s)?
(The other two failed jobs are probably just test flakes)

So I would hahaha, but I actually cannot install the dependencies on my machine

the error is this

error /Users/jonaskruckenberg/Documents/GitHub/sentry-javascript/node_modules/puppeteer: Command failed.
Exit code: 1
Command: node install.js
Arguments: 
Directory: /Users/jonaskruckenberg/Documents/GitHub/sentry-javascript/node_modules/puppeteer
Output:
The chromium binary is not available for arm64: 
If you are on Ubuntu, you can install with: 

 apt-get install chromium-browser

/Users/jonaskruckenberg/Documents/GitHub/sentry-javascript/node_modules/puppeteer/lib/cjs/puppeteer/node/BrowserFetcher.js:112
            throw new Error();
            ^

Error
    at /Users/jonaskruckenberg/Documents/GitHub/sentry-javascript/node_modules/puppeteer/lib/cjs/puppeteer/node/BrowserFetcher.js:112:19

seems like puppeteer is not supported on m1 Macs?

@JonasKruckenberg
Copy link
Contributor Author

Oh wait, do I have to install chrome myself?

@AbhiPrasad
Copy link
Member

@JonasKruckenberg I can try pushing to this branch if that is ok to you?

Also 🤔 not sure why there's an issue here - I can install just fine on my m1. Let me dig in!

@JonasKruckenberg
Copy link
Contributor Author

Sure thing! I also don't know why, but maybe it's because I have no chromium browser installed for puppeteer to fall back on?

@AbhiPrasad AbhiPrasad enabled auto-merge (squash) October 7, 2022 08:41
@AbhiPrasad AbhiPrasad merged commit 4bbfb2a into getsentry:master Oct 7, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants