feat(preprod): Show analysis URL after mobile-app upload

[chromy]

Adds output when uploading a mobile app to show the analysis URL:

$ sentry-cli mobile-app upload ~/apks/HackerNews.apk
> Preparing for upload completed in 0.229s
Successfully uploaded 1 file to Sentry
  - /Users/chromy/apks/HackerNews.apk https://sentry.my.sentry.io/sentry/preprod/internal/57

Also updates the sentry-cli logic for recent changes to the upload endpoint:

• gitSha renamed to headSha
• Endpoint also returns artifactId
• We now are not expected to poll till assemble is complete, instead we call assemble twice:
  • First time to find the missing chunks
  • (here we upload all missing chunks)
  • Second time to get the artfiactId and trigger an assembly job
    This avoids having to wait for the (slow) assembly process to complete prior to the CLI exiting.

[szokeasaurusrex]

The code looks alright to me. However, I am unsure we should implement this feature, see the inline comment about how this would require us to wait for assembly to finish.

If you have any questions, please feel free to reach out.

[szokeasaurusrex]

The disadvantage here is that because we require the artifact ID to be present in the response, we will always have to wait until the artifact has finished assembling, which could significantly slow down the command. It looks like we are already doing this, but in the future we may want to change it, as it is likely not the ideal behavior.

If we want to print the URL, there's probably no way around waiting for the assembly to finish. But, it is likely worth reconsidering whether we really think it is worth waiting for assembly to finish, just to be able to print the URL (with no way to opt in or out of this behavior)

[szokeasaurusrex]

Also, is the artifact_id always expected to be there if we hit this line (i.e., if the assembly status is OK)?

If not, I would suggest improving the error message to make it more actionable to the user.

If we do expect the artifact_id to be there, I would suggest we rewrite the error message to indicate that this is an internal error with the CLI and/or with the server, as it is not fixable by the user. You can do this by using expect rather than returning an error, as expect panics, and Sentry CLI's panic handler prints a message stating that an internal error has occurred, and that users should file a bug report

[szokeasaurusrex]

Edit: Not sure the above is accurate, I am currently reviewing #2683 and getsentry/sentry#97435 to verify this

Edit to the edit: I think my initial thoughts here were correct

[chromy]

We've updated the way this works so now the artifact id/url is available immediately.

[szokeasaurusrex]

This change would conflict with what @NicoHinderling is implementing in #2683, as I describe here, since #2683 removes the assembly polling, on which this PR depends

[chromy]

This change would conflict with what @NicoHinderling is implementing in #2683, as I describe here, since #2683 removes the assembly polling, on which this PR depends

Yes! Partly because of what you were saying (assembly can be slow) and also that assembly happens in a task so it's awkward to get the file.id out (turned out the query we were running was far to slow on us) we moved the creation of the artifactId up earlier into the process so now it's returned immediately. I also didn't get a chance to read all of the PRs get but I think broadly we want to land something like Nico's PR (#2683) first and then if anything is left to do I can rebase this one. I'll make this a draft for now.

[szokeasaurusrex]

@chromy just wanted to write it here so the discussion is preserved: after meeting with @NicoHinderling and understanding his changes better, I believe this PR should not conflict with his PR, once the needed changes are made on that PR and once this PR is rebased on it

[chromy]

@chromy just wanted to write it here so the discussion is preserved: after meeting with @NicoHinderling and understanding his changes better, I believe this PR should not conflict with his PR, once the needed changes are made on that PR and once this PR is rebased on it

Thanks! Nico and I have updated this one to hopefully take in to account all the feedback on both PRs (with the exception of having the server return a URL rather than an id which we would like to do in a follow up after this lands since it requires a server side change).

[loewenheim]

LGTM aside from some nits

[szokeasaurusrex]

I also left some suggestions, the rest looks good though!

[chromy]

Thanks both!