reorganizes index.js with more promises and error handling

[noslouch]

Hi

Sorry this is all in one commit, but it grew out of a spike I did in order to fix #17.

Sentry servers will return a 400 response to DELETE requests if there are actives issues on the requested release version.

{ detail: "This release is referenced by active issues and cannot be removed."}

Since _deleteRelease wasn't handling errors, _createRelease would reliably fail in these scenarios, since POST requests with existing version numbers would also return 400 errors, which would ultimately abort an entire deploy.

The general idea behind this patch is to allow a deploy to complete regardless if a release version exists in Sentry or not. Most of the time the release numbers are unique, whether generated via version numbers or hashes or both, so I'm not sure why there was an attempt to delete an existing release to begin with. It's possible I'm missing a use-case, but one thing this patch does is to remove that step from the code.

The flow goes like this:

* Make a request for a release with the given revision key for this deploy
* If that revision key exists, i.e. on 200, simply list the files known to sentry and finish
* If that revision key does not exist, i.e. on a 404, create a new release, more or less following your original flow

This also refactors a lot of the code to make it more "promise-like", so that we can avoid the "callback triangle".

Also this tweaks the getReleaseFiles method so that it outputs the filenames instead of [object Object].

[dschmidt]

Yes, you're missing a use case.

If you're redeploying an exisiting release, the old one needs to be deleted because otherwise the sourcemaps might conflict iirc. Can you check it's possible to delete all uploaded files instead? And in that case don't even try to create the release... Or can we maybe simply delete all assigned issues?

• If that revision key exists, i.e. on 200, simply list the files known to sentry and finish

I'm pretty unhappy with that. That would mean that you could never redeploy the revision .. if there were issues because of broken npm modules or something.

[noslouch]

Ah, I see. Yes, you should be able to upload new files to sentry under the same release name. I browsed the Sentry API docs and couldn't find anything about resolving issues remotely, but I like the idea of deleting and re-uploading if a release already exists.

Would you consider allowing add-on users to control this behavior using a config option? My use case doesn't require deleting and re-uploading files at deploy-time. Something like replaceFiles to control what happens if a release for the current revision number is found on Sentry?

[dschmidt]

Yes, sounds good.

[noslouch]

@dschmidt ready for another look

[dschmidt]

Awesome, looking great.

As this change is kinda invasive, I would like to give it a spin myself before I merge it tho, will do asap

[noslouch]

:bump:

[dschmidt]

I will review it once more today and if I don't manage to test it myself, I will just merge it... Sorry, for the delay

[dschmidt]

I know I could have asked this way earlier but somehow it slipped through ... can you undo the merge commits and rebase instead? Or if that is not possible cherry-pick all commits on top of a clean master branch and force push that to your branch? I really dislike merge commits because they obfuscate where changes are coming from and can easily introduce bugs in my experience

[noslouch]

@dschmidt all set

[dschmidt]

Why are logFiles and doUpload public?

[dschmidt]

Loving it even more than I remembered. Basically good to merge.

And again: Sorry for the delay and thanks for keeping this on your radar, you rock!

[noslouch]

TBH I'm not really sure where my head was at when considering which methods to mark as private v. public. I guess there's an argument to be made that only doesReleaseExist, handleExistingRelease, and createRelease should be marked public, since they're in the top level upload, so why don't I just make them private?

[dschmidt]

I'd argue that only functions called by Ember CLI deploy directly should be public - but don't worry, I've been nitpicky enough for now.

One last Question though: Isn't replaceFiles defaulting to false a backwards incompatible change? If someone relies on the release being deleted first and this just silently ignores already existing releases, this seems a bit bad :-\

Can we change the default for replaceFiles to true?

[noslouch]

Sure yeah. If someone wants to replace files if a match revision is found, then this default would change that behavior for them. I'll update.

[noslouch]

Updated

[dschmidt]

:clap: :clap: :clap: :tada: :tada: :tada: :clap: :clap: :clap:

Thanks a lot!

[noslouch]

thanks for merging! 😍

[noslouch]

this probably warrants a version bump. 0.4?

[dschmidt]

Yup, will release later tonight or tomorrow :+1: