search

JetBrains / teamcity-nuget-support

TeamCity NuGet support
Apache License 2.0
43 stars 11 forks source link

Fix crash on incorrect URI in NuGet trigger #2

Closed BenPhegan closed 11 years ago

BenPhegan commented 11 years ago

Hi,

This fix is related to an issue when a trigger is entered with an incorrect NuGet server URL. In this instance, it appears that the plugin will fail to check any packages that are provided in that run.

This simply check validity of the URI prior to creating an IQueryable set of packages, and alert via logging rather than blow up.

Also a tiny little typo fix.

Note: No tests are provided. I would be happy to add them, but first it would probably help if I had a chat about how they are currently set up and what the expectation for success are, as at the moment even without these changes I can only get 10 out of 124 tests to pass (28 ignored).

Regards,

Ben Phegan

jonnyzzz commented 11 years ago

Thanks for the patch. I believe it could be implemented in Java-side in order to report this are a trigger error. Have you created an issue in the tracker for that?

The fix in the .NET-side looks useful, but I supports it's better to fail on that and not continue.

Thanks for type fix!

jonnyzzz commented 11 years ago

The issue is http://youtrack.jetbrains.com/issue/TW-24105

BenPhegan commented 11 years ago

Hi Eugene,

It would help if it was in Java side as well, however the fix above does still provide a trigger error on the build that has an incorrect trigger, so you don't lose notification of failure on the Java side. Failing on the .Net side is very hard to troubleshoot without a lot of NuGet knowledge, so I don't think not fixing it is a good option. Best set of fixes I could imagine (for our use case)

  1. Fix so NuGet/.Net side will gracefully handle and return any checks it manages to do. Without this patch, any issue causing an exception for any source check will result in no checks being returned, even if there are 25 successful package checks and one that fails. This is pretty inefficient.
  2. Fix to trigger exception handling on the Java side so instead of just a failure at the build level and a stack trace, we get details on the package id/trigger that caused the failure as part of the message. The fix above allows you to narrow down this type of error to a single build, but you still have to check each and every trigger to see which is incorrect. Easy when there are only a few triggers, however we regularly have up to 30 triggers on a build type (I know, crazy right?).
  3. Fix to trigger source output on the Java side. This would ensure that the .Net side should not get bad URI values, but again you have instances where the source for a trigger is a parameter that can be provided at run time (or overriden) so we cannot rely on this not breaking the .Net side such that other triggers/sources are affected.

Java and I are not friends, but if I get time I will look into the Java side as well. But I really don't think that you can fix it only on the Java side.

Regards,

Ben

jonnyzzz commented 11 years ago

Got your point. I remember I added a code that issues a trigger error if there was 0 packages detected in the feed. Do you have an example of broken feed urls for me to update Java-side check?

BenPhegan commented 11 years ago

The value that tripped us up was something along the lines of "%env.NuG". Which should have been "%env.NuGetGalleryServer%". Pretty simple typo, but easy to make. I think that this is probably the most frequent one that we would see as all our triggers inherit from templates/projects.

jonnyzzz commented 11 years ago

I improved the change, please merge it back