dschmidt / ember-cli-deploy-sentry

An ember-cli-deploy-plugin to upload javascript sourcemaps to Sentry
MIT License
42 stars 51 forks source link

Make the options api consistent #57

Closed sumeetattree closed 5 years ago

sumeetattree commented 5 years ago

I've noticed that you are namespacing the keys twice. Because these are already inside ENV.sentry I think this is not required.

ENV.sentry = {
  publicUrl: 'https://your.awesome.site',
  sentryUrl: 'https://sentry.your.awesome.site',
  sentryOrganizationSlug: 'AwesomeOrg',
  sentryProjectSlug: 'AwesomeProject',
  sentryApiKey: 'awesomeApiKey',
  sentryBearerApiKey: 'awesomeApiKey'
}

May be change these to:

ENV.sentry = {
  // we could call assetHost as cdn but it won't make sense
  // for people using their own servers to host the js files
  assetHost: 'https://your.awesome.site', 
  url: 'https://sentry.your.awesome.site',
  organizationSlug: 'AwesomeOrg',
  projectSlug: 'AwesomeProject',
  apiKey: 'awesomeApiKey',
  bearerApiKey: 'awesomeApiKey'
}

While we are at it maybe we should remove the apiKey and call it something else? I am not sure what the old usage was like so I am at a loss here for suggestions. We should ideally rename bearerApiKey to apiKey.

Let me know if you'd like a pull request with these changes. Thanks!

dschmidt commented 5 years ago

I guess the idea was that I wanted to seperate sentry settings from settings that are more connected to your own project than to sentry (publicUrl). While I'm not exactly against it, I'm not sure it's worth the effort keeping things compatible and not breaking other people's pipeline...

sumeetattree commented 5 years ago

I get what you are saying. Maybe once you want to introduce a 2.0 you can take a look at the names again. I think I'll close this for now as this will do more harm than good at this point. Thanks!

dschmidt commented 5 years ago

Thanks for your effort anyway! :-)