Skip to content

feat: add dfx canister set-controller subcommand#1057

Merged
mergify[bot] merged 25 commits intomasterfrom
pshahi/setcontroller
Sep 28, 2020
Merged

feat: add dfx canister set-controller subcommand#1057
mergify[bot] merged 25 commits intomasterfrom
pshahi/setcontroller

Conversation

@p-shahi
Copy link
Contributor

@p-shahi p-shahi commented Sep 24, 2020

This adds the dfx canister set-controller subcommand

➜  hello git:(master) dfx canister set-controller --help
dfx-canister-set-controller
Sets the provided identity's name or its principal as the new controller of a canister on the Internet Computer network.

USAGE:
    dfx canister set-controller <canister> <new-controller>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <canister>          Specifies the canister name or the canister identifier for the canister to be controlled.
    <new-controller>    Specifies the identity name or the principal of the new controller.

An example use case:

➜  hello git:(master) dfx identity whoami
default
➜  hello git:(master) dfx identity list
default *
prithvi
➜  hello git:(master) dfx canister create hello
Creating canister "hello"...
"hello" canister created with canister id: "75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q"
➜  hello git:(master) dfx build hello
Building canisters...
➜  hello git:(master)
➜  hello git:(master) dfx canister set-controller hello prithvi
Set "prithvi" as controller of "hello".
➜  hello git:(master)
➜  hello git:(master) dfx canister install hello
Installing code for canister hello, with canister_id 75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q
Replica error (code 5): IC0512: Only the controller of canister 75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q can control it.
Expected controller: jxpuf-o4cfi-2zfqa-3svif-yxdiz-tuj3p-ctl6m-qsrfv-7zctw-z3bwc-2ae
Actual controller: 5gg4n-o6ooe-5zmlh-ac2kx-ntmkd-kamti-a3rqe-okecn-knil3-ord3t-mae

Note: Waiting for #1056 to get merged first (therefore, creating a PR against that branch)

@p-shahi p-shahi linked an issue Sep 24, 2020 that may be closed by this pull request
3 tasks
@p-shahi
Copy link
Contributor Author

p-shahi commented Sep 24, 2020

seeing same failure as PR 1056 when trying to enter nix-shell for e2e tests

error: failed to run custom build command for `brotli-sys v0.3.2`

Caused by:
  process didn't exit successfully: `/private/var/folders/l5/rhsc__9j79b_jrk2dl8k8d940000gp/T/nix-build-dfinity-sdk-rust-deps-unknown.drv-0/dummy-src/target/release/build/brotli-sys-1fc7cf0ca5ea697f/build-script-build` (exit code: 1)
--- stdout

@p-shahi p-shahi requested review from a user and lsgunnlsgunn September 24, 2020 18:05
@ghost
Copy link

ghost commented Sep 24, 2020

Was this discussed as a possibility?

dfx canister set-controller <canister> <new-controller>

instead of

dfx canister set-controller --canister <canister> --new-controller <new-controller>

Compare to

dfx canister call <canister> <method> <args..>

@p-shahi
Copy link
Contributor Author

p-shahi commented Sep 24, 2020

Was this discussed as a possibility?

dfx canister set-controller <canister> <new-controller>

instead of

dfx canister set-controller --canister <canister> --new-controller <new-controller>

Compare to

dfx canister call <canister> <method> <args..>

This format:

dfx canister set-controller <canister> <new-controller>

was how it was originally when I demo'd the command in the meeting. But the feedback was that explicitly specifying --canister <canister> --new-controller <new-controller> would be helpful (for scripting)

Adding .long("canister") or .long("new-controller") forces the user to specify it :(
and without .long() you can't specify --canister ---new-controller

➜  hello git:(master) dfx canister call --canister-name=hello greet everyone

error: Found argument '--canister-name' which wasn't expected, or isn't valid in this context

USAGE:
    dfx canister call [FLAGS] [OPTIONS] <canister_name> <method_name> [argument]

For more information try --help
➜  hello git:(master) dfx canister call --canister_name=hello greet everyone

error: Found argument '--canister_name' which wasn't expected, or isn't valid in this context

USAGE:
    dfx canister call [FLAGS] [OPTIONS] <canister_name> <method_name> [argument]

For more information try --help
➜  hello git:(master) dfx canister call canister_name=hello greet everyone

Cannot find canister id.  Please issue 'dfx canister create canister_name=hello'.
➜  hello git:(master) dfx canister call --canister_name hello greet everyone

error: Found argument '--canister_name' which wasn't expected, or isn't valid in this context

USAGE:
    dfx canister call [FLAGS] [OPTIONS] <canister_name> <method_name> [argument]

For more information try --help

I don't see a clap option to allow both

@p-shahi p-shahi requested a review from hansl September 24, 2020 18:32
Copy link
Contributor

@lsgunnlsgunn lsgunnlsgunn left a comment

Choose a reason for hiding this comment

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

LGTM-minor suggestion

@p-shahi
Copy link
Contributor Author

p-shahi commented Sep 24, 2020

though it was a suggestion during the demo of this command, adding the long form for the arguments i.e.
dfx canister set-controller --canister <canister> --new-controller <new-controller>
is inconsistent with dfx canister call <canister> <method> <args..>
it seems that arguments are "either positional or have a long form" (discussed this with eric on slack.) To be consistent with canister call, I think I should remove the long form, but I'd like your opinion @hansl @lsgunnlsgunn

@lsgunnlsgunn
Copy link
Contributor

lsgunnlsgunn commented Sep 24, 2020

Not sure if I understand your question ... are you suggesting to use this form to be consistent:
dfx canister set-controller <canister> <new-controller>
If so, I will admit that probably 50 to 60% of the time I get the dfx canister call command wrong on the first try because I forget to specify the canister name or get the method and name backwards. I'm not opposed to shortcuts, but 1) we aren't consistent about much of anything in terms of command syntax and 2) it is almost always worthwhile to support long-form names --dry-run, --mode) over only short form (-q) or unexpressed flags/switches. Experts can learn shortcuts but it is always better for novice, infrequent, and intermediate users to have long form descriptions (that's why it is considered a best practice in documentation to use the full syntax forms even for common options like --copy and --paste--so it always clear what operation is being performed. stepping off soap-box 😉 ``

@p-shahi
Copy link
Contributor Author

p-shahi commented Sep 24, 2020

Not sure if I understand your question ... are you suggesting to use this form to be consistent:
dfx canister set-controller <canister> <new-controller>
If so, I will admit that probably 50 to 60% of the time I get the dfx canister call command wrong on the first try because I forget to specify the canister name or get the method and name backwards. I'm not opposed to shortcuts, but 1) we aren't consistent about much of anything in terms of command syntax and 2) it is almost always worthwhile to support long-form names --dry-run, --mode) over only short form (-q) or unexpressed flags/switches. Experts can learn shortcuts but it is always better for novice, infrequent, and intermediate users to have long form descriptions (that's why it is considered a best practice in documentation to use the full syntax forms even for common options like --copy and --paste--so it always clear what operation is being performed. stepping off soap-box 😉 ``

I'm deferring to your judgement! I can see why the long-form is helpful but also can be verbose. Ideally i'd like to enable both, but it seems to be a limitation of the clap library we use

@p-shahi
Copy link
Contributor Author

p-shahi commented Sep 25, 2020

@lsgunnlsgunn talked with Hans, we're going with the short form and I updated the description to reflect that

@p-shahi p-shahi requested a review from a user September 28, 2020 16:19
Base automatically changed from hansl/update-agent to master September 28, 2020 20:03
@p-shahi p-shahi marked this pull request as ready for review September 28, 2020 20:13
@mergify mergify bot merged commit f5ebc00 into master Sep 28, 2020
@mergify mergify bot deleted the pshahi/setcontroller branch September 28, 2020 20:41
ghost pushed a commit that referenced this pull request Oct 6, 2020
This adds the `dfx canister set-controller` subcommand
```
➜  hello git:(master) dfx canister set-controller --help
dfx-canister-set-controller
Sets the provided identity's name or its principal as the new controller of a canister on the Internet Computer network.

USAGE:
    dfx canister set-controller <canister> <new-controller>

FLAGS:
    -h, --help       Prints help information
    -V, --version    Prints version information

ARGS:
    <canister>          Specifies the canister name or the canister identifier for the canister to be controlled.
    <new-controller>    Specifies the identity name or the principal of the new controller.
```
An example use case:
```
➜  hello git:(master) dfx identity whoami
default
➜  hello git:(master) dfx identity list
default *
prithvi
➜  hello git:(master) dfx canister create hello
Creating canister "hello"...
"hello" canister created with canister id: "75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q"
➜  hello git:(master) dfx build hello
Building canisters...
➜  hello git:(master)
➜  hello git:(master) dfx canister set-controller hello prithvi
Set "prithvi" as controller of "hello".
➜  hello git:(master)
➜  hello git:(master) dfx canister install hello
Installing code for canister hello, with canister_id 75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q
Replica error (code 5): IC0512: Only the controller of canister 75hes-oqbaa-aaaaa-aaaaa-aaaaa-aaaaa-aaaaa-q can control it.
Expected controller: jxpuf-o4cfi-2zfqa-3svif-yxdiz-tuj3p-ctl6m-qsrfv-7zctw-z3bwc-2ae
Actual controller: 5gg4n-o6ooe-5zmlh-ac2kx-ntmkd-kamti-a3rqe-okecn-knil3-ord3t-mae
```
Note: Waiting for #1056 to get merged first (therefore, creating a PR against that branch)

- [x] push local e2e tests which is blocked because of #1057 (comment)
- [ ] update to a replica version which doesn't expose the expected controller's principal
- [x] update to agent-rs after this is merged dfinity/agent-rs#51
dfinity-bot added a commit that referenced this pull request Sep 29, 2021
mergify bot pushed a commit that referenced this pull request Sep 29, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

dfx should have the ability to set controller per the public spec

3 participants