Skip to content
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.

Sign up for GitHub

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

Jump to bottom

daml script runner fails sooner if provided ledger host lacks a protocol when using the JSON API #19175

Merged
merged 6 commits into from May 10, 2024
Merged

daml script runner fails sooner if provided ledger host lacks a protocol when using the JSON API #19175

merged 6 commits into from May 10, 2024

Conversation

moisesackerman-da
Copy link
Contributor

Fixes #19174

@moisesackerman-da moisesackerman-da requested a review from a team May 10, 2024 10:45
@moisesackerman-da moisesackerman-da changed the base branch from main to main-2.x May 10, 2024 10:45
@moisesackerman-da moisesackerman-da changed the title daml script runner fails sooner if provided ledger host lacks a protocol daml script runner fails sooner if provided ledger host lacks a protocol when using the JSON API May 10, 2024
garyverhaegen-da
garyverhaegen-da approved these changes May 10, 2024
View reviewed changes
Copy link
Contributor

@garyverhaegen-da garyverhaegen-da left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you!

@garyverhaegen-da
Copy link
Contributor

Would this be worth backporting to 2.8?

@moisesackerman-da
Copy link
Contributor Author

Would this be worth backporting to 2.8?

yes I think so, perhaps even 2.7.x and 2.3.x too; the use of Uri(_) goes way back - see #9057, first released with Daml SDK 1.12.0, which clarified a few of the daml script options, mentions that --ledger-host requires a protocol if using the JSON API. Ofc if the presumably few users of the older SDKs haven't complained so far it's unlikely they will complain at all, so I'll leave the backporting to you

moisesackerman-da
moisesackerman-da commented May 10, 2024
View reviewed changes
sdk/daml-script/runner/src/test/resources/TestScript.daml
myScript : Script ()
myScript = do
_ <- getTime
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

these tests would have caught the missing protocol errors if it hadn't been for the fact that the default time mode is --wall-clock-time, which does not call to the ledger :(

@moisesackerman-da moisesackerman-da merged commit 364deff into main-2.x May 10, 2024
17 checks passed
@moisesackerman-da moisesackerman-da deleted the akrmn/fix-19174 branch May 10, 2024 15:17
garyverhaegen-da pushed a commit that referenced this pull request May 10, 2024
@moisesackerman-da @garyverhaegen-da
[backport #19175 to 2.9] script fails on missing protocol for JSON API (
5ee724c 
#19175)

See #19175.
garyverhaegen-da added a commit that referenced this pull request May 10, 2024
@garyverhaegen-da
[backport #19175 to 2.8] script fails on missing protocol for JSON API (
0066568 
#19175)

See #19175.
@garyverhaegen-da garyverhaegen-da mentioned this pull request May 10, 2024
Merged
garyverhaegen-da added a commit that referenced this pull request May 13, 2024
@garyverhaegen-da
[backport #19175 to 2.8] script fails on missing protocol for JSON API (
d097971 
#19175) (#19178)

See #19175.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@garyverhaegen-da garyverhaegen-da garyverhaegen-da approved these changes

@gerolf-da gerolf-da Awaiting requested review from gerolf-da

@remyhaemmerle-da remyhaemmerle-da Awaiting requested review from remyhaemmerle-da

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[support-6918] daml-script --json-api gives confusing error when --ledger-host lacks protocol
2 participants
@moisesackerman-da @garyverhaegen-da