fix flux plugin UpdateInstalledPackage() according to agreed upon semantics #3468
Conversation
| return fluxPluginClient | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
Thanks - yes, now it explicitly fails if someone tries to enable them but hasn't got the kind cluster running etc.
| // global vars | ||
| var letters = []rune("abcdefghijklmnopqrstuvwxyz0123456789") | ||
| var typedClient kubernetes.Interface | ||
| var dynamicClient dynamic.Interface |
There was a problem hiding this comment.
Do we need global vars? Normally it should be possible to avoid this type of state (your kubeGet* fns above already return the clients).
There was a problem hiding this comment.
not sure I get what you're asking. What's the alternative to global vars if I want to keep some state in between calls (other than to have to declare a new receiver struct or explicitly pass in the state back and forth, yuck)? Must be some language feature I am not aware of. Please tell me.
There was a problem hiding this comment.
I'm not asking you to change anything here, but worth reading: https://dave.cheney.net/2017/06/11/go-without-package-scoped-variables
There was a problem hiding this comment.
Sure, I'll give it a read. Just so we are on the same page, this is test code we are talking about, not production. Furthermore, this is test code that you've pointed out will become obsolete at some point not too far away. So not going an extra mile (or in this case an extra 10 miles based on what I read of the article so far) and taking a shortcut seemed like an acceptable thing to do for that case. If you find any cases where I am doing it in production code, please point them out
There was a problem hiding this comment.
Yep, as above, nothing to change here. Just worth being aware, that's all. I like the last line ("the bottom line") of Dave's post.
| expectedRelease: flux_helm_release_updated_1, | ||
| }, | ||
| { | ||
| name: "returns invalid if installed package doesn't exist", |
There was a problem hiding this comment.
yes, indeed. I copied that case from your test, so you might want to correct that one. hehe
| @@ -50,7 +50,7 @@ service PackagesService { | |||
|
|
|||
| rpc UpdateInstalledPackage(UpdateInstalledPackageRequest) returns (UpdateInstalledPackageResponse) { | |||
| option (google.api.http) = { | |||
| patch: "/core/packages/v1alpha1/installedpackages" | |||
| put: "/core/packages/v1alpha1/installedpackages" | |||
There was a problem hiding this comment.
Nice, so there will be no problem with us later providing a patch method here either (though I understand the message formats will need references throughout).
No description provided.