storage History function has inconsistent behaviours depending on the driver

[mikeshng]

The storage History function states returns ErrReleaseNotFound if no such release name exists.
https://github.com/helm/helm/blob/master/pkg/storage/storage.go#L150

For secret and cfgmap, it's possible to have an empty return because there is no length check.
https://github.com/helm/helm/blob/master/pkg/storage/driver/secrets.go#L129-L138
https://github.com/helm/helm/blob/master/pkg/storage/driver/cfgmaps.go#L138-L147

For memory and sql, it's not possible to return an empty due to a length check.
https://github.com/helm/helm/blob/master/pkg/storage/driver/memory.go#L145-L146
https://github.com/helm/helm/blob/master/pkg/storage/driver/sql.go#L313-L325

This seems to be inconsistent behaviour. Is this intentional? Thanks.

Output of helm version:

version.BuildInfo{Version:"v3.1.2", GitCommit:"d878d4d45863e42fd5cff6743294a11d28a9abce", GitTreeState:"clean", GoVersion:"go1.13.8"}

Output of kubectl version:

Client Version: version.Info{Major:"1", Minor:"18", GitVersion:"v1.18.2", GitCommit:"52c56ce7a8272c798dbc29846288d7cd9fbae032", GitTreeState:"clean", BuildDate:"2020-04-16T11:56:40Z", GoVersion:"go1.13.9", Compiler:"gc", Platform:"linux/amd64"}
Server Version: version.Info{Major:"1", Minor:"16", GitVersion:"v1.16.4", GitCommit:"224be7bdce5a9dd0c2fd0d46b83865648e2fe0ba", GitTreeState:"clean", BuildDate:"2020-01-14T01:25:43Z", GoVersion:"go1.12.12", Compiler:"gc", Platform:"linux/amd64"}

Cloud Provider/Platform (AKS, GKE, Minikube etc.):
kind

[bacongobbler]

Hi @mikeshng. Please have a look at the PR queue. #7372 appears to address this case for the memory driver, but perhaps there's some feedback the author needs here with regards to the postgresql driver. Please check and confirm if this PR will fix this issue for you. Thanks.

[bacongobbler]

#7372 (comment) might also be relevant here. Feedback welcome on how we can standardize this behaviour across all drivers moving forward.

[mikeshng]

@bacongobbler I agree with the memory driver change. But the sql driver deserves a second look.

The check for sql driver should change from

	if len(releases) == 0 {
		return nil, ErrReleaseNotFound

to

	if len(records) == 0 {
		return nil, ErrReleaseNotFound

so that it's consistent with secret and cfgmap.

I think if the release exists, but all the decoding fails and its going to return an empty array then that should be a new error (not ErrReleaseNotFound) but something that tells the user, release exists but it fails to decode it. Maybe the comment for History can mention the fact that empty return doesn't mean the release doesn't exists. What do you think?

I am also wondering about the differences betweenGet vs List vs Query. Get seems to return the decode error, I guess this is intentional? Another minor difference the secret Get decode error is wrapped and the cfgmap Get decode error is return directly.

[bacongobbler]

Please provide feedback to the author. I'm just the messenger here. :)

[hickeyma]

@mikeshng Thanks for raising this issue and showing the inconsistency. I agree that all drivers should follow the same pattern.

As the secret and configmap drivers predate sql and memory drivers, I would be more inclined to follow its pattern and leave the checking to the caller of the storage layer.

@mikeshng @absoludity How about one PR from #7372 and #8470 which removes the length checks in the SQL and memory drivers and returns the release as is? I would prefer it to merge all in one.

[mikeshng]

@hickeyma Thanks for your feedback.

The main issue in #7372 its already merged in by another commit 1ab52fa#diff-77d70768631351624c46343a0d158658

That's why I created #8470 because I thought the other PR is already stale.

[hickeyma]

@mikeshng Ok but it would still be better to sync with @absoludity first. I would like to get agreement before progressing on.

[absoludity]

I agree - standardizing on ErrReleaseNotFound to mean that no releases were found at all (which is what it means for the secret and configmap drivers and now memory driver), not that some were found but couldn't be parsed, is a good thing

I think if the release exists, but all the decoding fails and its going to return an empty array then that should be a new error (not ErrReleaseNotFound) but something that tells the user, release exists but it fails to decode it.

I also think this would be clearer (and a better, less confusing experience, for the corner case where a release can't be parsed). I'll leave a few comments related to this in #8470

[absoludity]

I think if the release exists, but all the decoding fails and its going to return an empty array then that should be a new error (not ErrReleaseNotFound) but something that tells the user, release exists but it fails to decode it.

So regarding this possible change in a subsequent PR, we could perhaps define a more general error:

const QueryReleaseError = errors.New("query release error")

and then wrap this for ErrReleaseNotFound as well as the new ErrReleaseCannotBeParsed or similar. This way tests and external code, can use errors.Is rather than checking for the error text like the referenced commit?

Thanks for syncing with me first :)

[hickeyma]

and then wrap this for ErrReleaseNotFound as well as the new ErrReleaseCannotBeParsed or similar. This way tests and external code, can use errors.Is rather than checking for the error text like the referenced commit?

I like this @absoludity. So, update the implementation of Query function for each of the drivers to return ErrReleaseNotFound for empty return from storage and new ErrReleaseCannotBeParsed for parsing error.

[github-actions]

This issue has been marked as stale because it has been open for 90 days with no activity. This thread will be automatically closed in 30 days if no further activity occurs.