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.

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

3873 z_setmigration cli bool enable arg conversion #3977

Merged
merged 1 commit into from May 1, 2019

Conversation

@LarryRuane
Copy link
Contributor

commented Apr 30, 2019

Addresses #3873.

@LarryRuane LarryRuane self-assigned this Apr 30, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

Without this fix:

$ z_setmigration false
error code: -1
error message:
JSON value is not a boolean as expected
$ 

Discovered this by manual testing -- the Python tests do json conversion automatically, so the RPC works there.

@LarryRuane LarryRuane requested review from bitcartel, daira and Eirik0 Apr 30, 2019

@LarryRuane LarryRuane self-assigned this Apr 30, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented Apr 30, 2019

There is no workaround that I know of; I tried writing the argument as 1 instead of true and that doesn't work either (same error message). so I think this RPC is not usable without this fix.

@str4d

str4d approved these changes Apr 30, 2019

Copy link
Contributor

left a comment

utACK

@Eirik0

Eirik0 approved these changes Apr 30, 2019

Copy link
Contributor

left a comment

ACK

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented Apr 30, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

📌 Commit 222b7bd has been approved by Eirik0

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented Apr 30, 2019

⌛️ Testing commit 222b7bd with merge e438861...

zkbot added a commit that referenced this pull request Apr 30, 2019

Auto merge of #3977 - LarryRuane:3873-setmigration-cli, r=Eirik0
3873 z_setmigration cli bool enable arg conversion

Addresses #3873.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

The failure(s) is/are not related to this PR:

./zcutil/fetch-params.sh: line 161: cannot create temp file for here-document: No space left on device
@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 222b7bd with merge 4ea608d...

zkbot added a commit that referenced this pull request May 1, 2019

Auto merge of #3977 - LarryRuane:3873-setmigration-cli, r=Eirik0
3873 z_setmigration cli bool enable arg conversion

Addresses #3873.

@Eirik0 Eirik0 added this to Needs Prioritization in Arborist Team via automation May 1, 2019

@Eirik0 Eirik0 moved this from Needs Prioritization to Merge Queue in Arborist Team May 1, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@zkbot retry

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 222b7bd with merge 0c4f95f...

zkbot added a commit that referenced this pull request May 1, 2019

Auto merge of #3977 - LarryRuane:3873-setmigration-cli, r=Eirik0
3873 z_setmigration cli bool enable arg conversion

Addresses #3873.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

💔 Test failed - pr-merge

@daira

daira approved these changes May 1, 2019

Copy link
Contributor

left a comment

utACK

@daira

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Still a timeout, probably not related to the PR.

@zkbot retry

zkbot added a commit that referenced this pull request May 1, 2019

Auto merge of #3977 - LarryRuane:3873-setmigration-cli, r=Eirik0
3873 z_setmigration cli bool enable arg conversion

Addresses #3873.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 222b7bd with merge 098001d...

@str4d str4d added this to the v2.0.5 milestone May 1, 2019

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

💔 Test failed - pr-merge

@Eirik0

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

Is this an actual failure? It seems to be happening consistently.

@@ -130,7 +130,8 @@ static const CRPCConvertParam vRPCConvertParams[] =
{ "z_importkey", 2 },
{ "z_importviewingkey", 2 },
{ "z_getpaymentdisclosure", 1},
{ "z_getpaymentdisclosure", 2}
{ "z_getpaymentdisclosure", 2},
{ "z_setmigration", 0},

This comment has been minimized.

Copy link
@Eirik0

Eirik0 May 1, 2019

Contributor

I notice that before this list did not have a comma in anticipation of the next item but now it does. I know we do this in other languages, but I don't remember it being a standard thing in cpp. I am not sure if this could be related to the failures, but it is something that is different now.

This comment has been minimized.

Copy link
@LarryRuane

LarryRuane May 1, 2019

Author Contributor

It is okay, allowed in cpp. It should have no relation to the failures.

Can we wait to r-plus this again, I just wrote a unit test for this change, almost ready to force-push.

@LarryRuane LarryRuane force-pushed the LarryRuane:3873-setmigration-cli branch from 222b7bd to 4cbe0a9 May 1, 2019

@LarryRuane

This comment has been minimized.

Copy link
Contributor Author

commented May 1, 2019

The most recent force-push adds unit tests (no functional change).

@bitcartel
Copy link
Contributor

left a comment

ACK (change and test)

@Eirik0

Eirik0 approved these changes May 1, 2019

Copy link
Contributor

left a comment

reACK

@bitcartel

This comment has been minimized.

Copy link
Contributor

commented May 1, 2019

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

📌 Commit 4cbe0a9 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

⌛️ Testing commit 4cbe0a9 with merge d894f7b...

zkbot added a commit that referenced this pull request May 1, 2019

Auto merge of #3977 - LarryRuane:3873-setmigration-cli, r=bitcartel
3873 z_setmigration cli bool enable arg conversion

Addresses #3873.
@zkbot

This comment has been minimized.

Copy link
Collaborator

commented May 1, 2019

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing d894f7b to master...

@zkbot zkbot merged commit 4cbe0a9 into zcash:master May 1, 2019

1 check passed

homu Test successful
Details

Arborist Team automation moved this from Merge Queue to Released (Merged in Master) May 1, 2019

@str4d str4d referenced this pull request May 3, 2019

Open

Automated smoke tests #3913

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.