-
Notifications
You must be signed in to change notification settings - Fork 29
Expose ledger API in docker compose setup #2053
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Required for #2053 to go through CI. [static] Signed-off-by: Moritz Kiefer <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, thank you. Shouldn't we also add it to localnet?
localnet already has support |
Required for #2053 to go through CI. [static] Signed-off-by: Moritz Kiefer <[email protected]> Co-authored-by: Moritz Kiefer <[email protected]>
Hm looking at this again, localnet does it differently though.
Don't see what the point of this is given that nothing else uses the same server name. The inconsistency does seem awkward. I think I have a slight preference for the setup here so I'm leaning towards adjusting localnet to the urls here (but with cors) and also expose the grpc API there. I guess this is technically a breaking change but it seems trivial to adjust wdyt? |
@isegall-da Can I ask your input on the proposal above to adjust localnet to match the |
So the main difference seems to be that in localnet we made an effort not to expose everything, but only I tend to agree that otherwise, the proposal in this PR seems nicer. I guess that in localnet we can have both for a while, and label the existing deprecated, no? |
there isn't anything else. I think the filter is probably copypasta from actual UI configs. There it's common you want those filters so / serves your static UI files and then /v2 serves the json API. But here it is on a different port so this doesn't do anything useful.
Yeah realized that afterwards. No need to make it a breaking change so I'll just keep the exisitng one with a comment. |
fixes #2038 [ci] This keeps coming up every few days so rather than copy pasting the config snippet around let's just include it. Signed-off-by: Moritz Kiefer <[email protected]>
[ci] Signed-off-by: Moritz Kiefer <[email protected]>
761f597
to
c5c4d9e
Compare
[ci] Signed-off-by: Moritz Kiefer <[email protected]>
[ci] Signed-off-by: Moritz Kiefer <[email protected]>
[ci] Signed-off-by: Moritz Kiefer <[email protected]>
fixes #2038
[ci]
This keeps coming up every few days so rather than copy pasting the config snippet around let's just include it.
Pull Request Checklist
Cluster Testing
/cluster_test
on this PR to request it, and ping someone with access to the DA-internal system to approve it./hdm_test
on this PR to request it, and ping someone with access to the DA-internal system to approve it.PR Guidelines
Fixes #n
, and mention issues worked on using#n
Merge Guidelines