feat!: Replace actions env secret endpoints#4335
Conversation
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
gmlewis
added
NeedsReview
| } | ||
|
|
||
| func encryptSecretWithPublicKey(publicKey *github.PublicKey, secretName, secretValue string) (*github.EncryptedSecret, error) { | ||
| func encryptSecretWithPublicKey(publicKey *github.PublicKey, secretName, secretValue string) (github.EncryptedSecret, error) { |
There was a problem hiding this comment.
What value is there to changing the return type from a pointer to a value?
It is pretty standard practice in Go to return nil when there is an error.
Why would we change that for this method?
There was a problem hiding this comment.
I'm not sure it makes any difference in this code if we're returning nil or an empty struct, AFAIK both could be considered idiomatic Go. I did this just before logging off for the weekend so I'm on my phone and can't see if this is a linter error? If it is I'll fix it. FYI I don't this example is particularly idiomatic Go given the unnecessary function separation.
There was a problem hiding this comment.
Yeah, there are linter and test errors in this PR.
During code reviews at Google, I was told to not return a value struct during an error because a nil value was perfectly valid. So from my experience with the Go team, it is more idiomatic to return a nil pointer to a struct than to return an empty value struct when errors are possible.
There was a problem hiding this comment.
Thanks, I'll fix it up when I get a few mins. I'll probably just get rid of the function call as that'd remove the whole issue.
2 participants
This PR updates the actions environment secrets functions to use the documented and supported endpoints, it also changes all actions secret create/update functions to use a value parameter (#3644).