Support exploring non-default KV stores #27
Conversation
kate-goldenring
force-pushed
the
explore-store-by-label
branch
from
28af926 to
2f25686
Compare
kate-goldenring
force-pushed
the
explore-store-by-label
branch
from
2f25686 to
d8b6663
Compare
|
@karthik2804 @radu-matei @calebschoepp, I do not have the permissions in this repo to add reviewers, but your reviews would be helpful given your previous work in the repo. @karthik2804 and i discussed how in the future we may want one "explorer" for all resources (KV and SQL). For now, this gets KV to links parity |
explorer/main.go
Outdated
| w.WriteHeader(http.StatusInternalServerError) | ||
| fmt.Fprintf(w, "KV explorer credentials not configured.\n") | ||
| // Log more informative error in cloud logs | ||
| fmt.Printf("Error getting KV explorer credentials from config store: %v\n", err) |
There was a problem hiding this comment.
I believe this line will be printed twice. Once within the GetCredentials function and the bubbled-up error handling here.
There was a problem hiding this comment.
I wasn't sure about this one. I see the second line as a log, but logging doesn't work right now for Go, so instead i used a print. The first i see as additional context to the error. If we just do one, I'd prefer the second, since sometime the error is captured by the front end and never displayed in cloud logs.
There was a problem hiding this comment.
I was trying to point at line 76 and then line 239. I also just noticed that the error message would be of the form:
Error getting KV explorer credentials from config store: cannot get credentials pair from config store: {some_err}
There was a problem hiding this comment.
Thanks! Removed the line to avoid the error duplication
explorer/main.go
Outdated
| if err != nil { | ||
| return "", "", fmt.Errorf("cannot get credentials pair from store: %v", err) | ||
| fmt.Printf("ERROR: cannot get credentials pair from config store: %v\n", err) | ||
| fmt.Printf("Set the 'kv_explorer_user' and 'kv_explorer_password' variables using 'spin cloud variables set'.\n") |
There was a problem hiding this comment.
nit: could this potentially be confusing to local spin users?
There was a problem hiding this comment.
These are logs. They are displayed in the cloud logs not in the UI. The error is caught in the front end and the associated message is here: https://github.com/kate-goldenring/spin-kv-explorer/blob/d8b666365e72552dc3734f3f5bba48f6461810a4/explorer/index.html#L314-L315:
alert(`Access denied to store with label "${storeLabel}." Ensure the app is linked to the store with this label and that the label has been specified in the Spin manifest (spin.toml).`);
There was a problem hiding this comment.
They'll be printed to the local console under spin up though won't they?
There was a problem hiding this comment.
As @itowlson points out, that is part of the local spin I was trying to point at.
There was a problem hiding this comment.
I see -- that's a great point. I was hyper focused on the Cloud scenario. Are you suggesting we remove this line? :
fmt.Printf("Set the 'kv_explorer_user' and 'kv_explorer_password' variables using 'spin cloud variables set'.\n")
There was a problem hiding this comment.
Suggestion:
"The [correct variables] for the application are not set. For local testing, you can disable authentication using the [skip auth variable], or set them in you application using [link to setting app variables for all scenarios running Spin]"?
There was a problem hiding this comment.
I updated the log to give directions for both local and deployed apps:
The 'kv_explorer_user' and 'kv_explorer_password' variables for the application are not set. For deployed applications, set the variables using 'spin cloud variables set'. For local development, you can disable authentication using '--env SPIN_APP_KV_SKIP_AUTH=1' or set them in the application using runtime configuration (https://developer.fermyon.com/spin/v2/dynamic-configuration#application-variables-runtime-configuration)
|
@kate-goldenring the engineering team on GH should now have write access to the repo (that was an oversight when we moved the repo recently), so you should be able to add reviewers. |
|
Not sure it's caused by this PR, but I'm noticing it's not impossible to skip checking the auth when running locally — so we either want to remove it (and update the docs), or decide if it's still valuable? |
Signed-off-by: Kate Goldenring <kate.goldenring@fermyon.com>
kate-goldenring
force-pushed
the
explore-store-by-label
branch
from
d8b6663 to
249a84a
Compare
@radu, it was caused by this PR. Thanks for catching that. Just pushed a fix |
Supports exploring non-default KV stores by adding a top level field to specify the label of which store to explore
Stores the user/pass credentials in the app config store instead of in the "default" KV store
Draft as there is currently an issue getting the template to work with variables<< Thanks @itowlson!Thank you @karthik2804 for help with the UI!!
To test it, follow updated README instructions
Associated template updates: #28