Return cloud-credentials with empty attribute #1333
Conversation
internal/jujuapi/cloud.go
Outdated
| @@ -321,7 +321,7 @@ func getIdentityCredentials(ctx context.Context, user *openfga.User, j JIMM, arg | |||
| } | |||
| var err error | |||
| content.Attributes, _, err = j.GetCloudCredentialAttributes(ctx, user, c, args.IncludeSecrets) | |||
There was a problem hiding this comment.
thinking out loud...
now this map is set to nil, and if you try to access it it will panic, i would prefer j.GetCloudCredentialAttributes to return an empty map without any error. It is even more correct, because we don't want to treat empty attributes as an error.
What do you think?
internal/jujuapi/cloud.go
Outdated
| @@ -322,7 +322,11 @@ func getIdentityCredentials(ctx context.Context, user *openfga.User, j JIMM, arg | |||
| var err error | |||
| content.Attributes, _, err = j.GetCloudCredentialAttributes(ctx, user, c, args.IncludeSecrets) | |||
| if err != nil { | |||
| return nil, errors.E(err) | |||
| if errors.ErrorCode(err) == errors.CodeNotFound { | |||
There was a problem hiding this comment.
Maybe it is even better to return an empty map without any error here:
https://github.com/kian99/jimm/blob/3fbc98f6bec1583090243b8862e7a25c4c25cdbe/internal/jimm/cloudcredential.go#L380
There was a problem hiding this comment.
Yeah true actually! Good point
There was a problem hiding this comment.
I've updated this, seems like a nil map is treated like a nil slice, in the sense that using a nil map e.g. range myMap won't cause a panic even though it is nil.
There was a problem hiding this comment.
yes, but this will
https://go.dev/play/p/nBBELjOwxnj
There was a problem hiding this comment.
Yeah good point, fixed.
* return cloud-credentials with empty attribute * set empty map if attributes not found * change application logic to not return error on empty attributes * add app layer test * return empty map rather than nil * fix test
Description
An issue was found when testing the Juju Terraform provider against JIMM. When requesting a cloud-credential, if the cloud-credential had an empty attribute map, JIMM would return a "not found" error even though the cloud-credential was found.
This is different behaviour from Juju where you can retrieve your cloud-credential even if the attribute map is empty. This 1 line change fixes that and additionally adds a test for it.
Engineering checklist
Check only items that apply