Support multiple resources #48
Conversation
| @@ -199,7 +296,7 @@ func (c installer) syncReleaseStatus(status v1alpha1.HelmAppStatus) { | |||
|
|
|||
There was a problem hiding this comment.
Not part of this PR but just noticed that syncReleaseStatus doesn't return an error while c.storageBackend.Create(status.Release) could on line 294.
From the header comment for storageBackend.Create
An error is returned if the storage driver failed to store the release, or a release with identical an key already exists.
Are we purposefully ignoring the error if the release already exists? The storage driver could fail for some other reason as well.
There was a problem hiding this comment.
Yeah I think I caught that in the original PR. I've got another PR coming on the heels of this one that will fix that.
| c.syncReleaseStatus(r.Status) | ||
|
|
||
| status := v1alpha1.StatusFor(r) | ||
| c.syncReleaseStatus(*status) | ||
|
|
||
| if err != nil || latestRelease == nil { |
There was a problem hiding this comment.
Also not a change from this PR but shouldn't this check be done directly after line 206:
tiller := tillerRendererForCR(r, c.storageBackend, c.tillerKubeClient)
latestRelease, err := c.storageBackend.Last(releaseName(r))
if err != nil || latestRelease == nil {
...
} else {
...
}
status := v1alpha1.StatusFor(r)Functionally it's the same but it's a bit more readable.
There was a problem hiding this comment.
Agreed. And also fixed in the next PR.
|
@joelanford overall LGTM after nits. We don't have an e2e test for this in the CI right? Something similar to the ansible-operator e2e tests where we build and run the helm-app-operator for a sample watches.yaml and chart directory. |
|
Correct - no e2e tests yet, but definitely needed. Added new issue #50 |
This PR mostly (but not completely) supersedes #43. It:
lastResourceVersion.Each of these changes is contained in its own commit on this branch.