Use dry-run to resolve manifest when rolling back

[andresmgot]

Follow up #997

After the feedback from @absoludity I checked that it's actually not necessary to send the chart details to tiller-proxy in order to retrieve the chart manifest that a release has.

We can get the changes we are going to perform if we run the rollback with the dry-run option.

I tested this manually and it works but there is not anything useful to test here since we are depending on the result from tiller.

[absoludity]

Follow up #997

After the feedback from @absoludity I checked that it's actually not necessary to send the chart details to tiller-proxy in order to retrieve the chart manifest that a release has.

Great! Nice to see lots of code removed :)

We can get the changes we are going to perform if we run the rollback with the dry-run option.

Is this what helm get manifest does?

I tested this manually and it works but there is not anything useful to test here since we are depending on the result from tiller.

Hmm - I know what you mean... regarding testing the ResolveManifestFromRelease function, it doesn't seem useful to write a test which just confirms that a call to ResolveManifestFromRelease calls the fake proxy, trims and returns the result... but IMO the test isn't just to test the code you're adding here, it's also to make it easier for someone (possibly you :) ) in the future to modify that function and confirm it still behaves as expected. I'm +1 on adding a test (if you're keen, it's a chance to write a small table-based test showing that it will error if the release isn't found, and return the correct result if it is etc.) But not blocking on this :)

Regarding the handler test - I'm surprised existing handler tests don't fail - there seems to be tests there supporting rollbacks with chart details, yet you've replaced that code to instead just use the release? Even if they are validly passing (because the info is just not used) they should be updated to avoid confusion in the future imo.

[andresmgot]

Is this what helm get manifest does?

They actually use a different method (but the result is the same):

https://github.com/helm/helm/blob/b0b0accdfc84e154b3d48ec334cd5b4f9b345667/cmd/helm/get_manifest.go#L75

I guess it makes more sense to use that.

I'm +1 on adding a test (if you're keen, it's a chance to write a small table-based test showing that it will error if the release isn't found, and return the correct result if it is etc.) But not blocking on this :)

Yes, I tried that. The problem was that the fake method to rollback the release didn't return anything so trying to access the manifest caused a segmentation fault. In the other hand, using ReleaseContent the fake behavior is better so I can test this.

Regarding the handler test - I'm surprised existing handler tests don't fail - there seems to be tests there supporting rollbacks with chart details, yet you've replaced that code to instead just use the release? Even if they are validly passing (because the info is just not used) they should be updated to avoid confusion in the future imo.

Yes, I missed that.