Adds metastore endpoints#65
Conversation
0c2e495 to
0b7f699
Compare
92a46b4 to
817f96a
Compare
| , permissions ∷ Array TokenHash | ||
| | r | ||
| } | ||
| type Config r = QuasarF.Config ( idToken ∷ Maybe IdToken, permissions ∷ Array TokenHash | r ) |
There was a problem hiding this comment.
This is a question, what happened to basePath, I saw you put all this into one place from QuasarAF/Interpreter/Aff.purs which makes sense but then didn't understand if basePath was still needed 😄
There was a problem hiding this comment.
from Quasar.QuasarF.Interpreter.Config:
type Config r = { basePath ∷ BasePath | r }
so we're just extending that here, which means we still have the basePath.
garyb
left a comment
There was a problem hiding this comment.
The only thing I think that actually needs changing here is the additions to Quasar.Advanced.QuasarAF, but I left a few other points for possible discussion.
| show = case _ of | ||
| H2Metastore s → "(H2Metastore " <> show s <> ")" | ||
| PostgresMetastore { host, port, database } → | ||
| "(PostgresMetastore { host: " <> show host <> ", port: " <> show port <> ", database: " <> show database <> "})" |
There was a problem hiding this comment.
Could derive generic and then show for this? I've been moving towards that in here, since show instances are the worst. 😄
There was a problem hiding this comment.
Oh, I guess not because of the extensible row.
| ~> "password" := password | ||
| ~> jsonEmptyObject | ||
| ) | ||
| ~> jsonEmptyObject |
There was a problem hiding this comment.
Can we use the codec stuff for this, or is it inconvenient to get the exact format without resorting to basicCodec anyway or something?
There was a problem hiding this comment.
Again, I see, it's two different codecs because of the extensible row.
| toJSON ∷ Config → Json | ||
| toJSON config = | ||
| let uri = URI.printAbsoluteURI (toURI config) | ||
| let (uri ∷ String) = AbsoluteURI.print (toURI config) |
There was a problem hiding this comment.
Is the annotation necessary here? I thought AbsoluteURI.print was monomorphic.
| putMetastore | ||
| ∷ { initialize ∷ Boolean, metastore ∷ Metastore (password ∷ String) } | ||
| → QuasarFE Unit | ||
| putMetastore ms = PutMetastore ms id |
There was a problem hiding this comment.
We should add the appropriate versions of these in Quasar.Advanced.QuasarAF too.
|
I think I addressed your feedback. We might want to coordinate merging and releasing this, since it requires everything in slamdata to be compatible with purescript-uri 4.0.0. (for example: slamdata/purescript-leafletjs#5) |
|
Btw, does this need changing with regards to https://github.com/quasar-analytics/quasar/pull/2767/files? |
|
Hmm, good point. Have you tried using it / can we add a test case for it perhaps? |
85160f0 to
92cd17c
Compare
Also renders URLs through purescript-uri instead of munging together strings
also adds a simple smoke test
92cd17c to
6b6d01b
Compare
6b6d01b to
a3cdb46
Compare
Also renders URLs through purescript-uri instead of munging together strings
I can't figure out how to make the tests go locally, so I hope they work on Travis