SDK-607 feat: replace using IDs on the command line with project name #145

hansl · 2019-11-07T03:40:03Z

This includes install which does not take a wasm path anymore.

We have a new structure for getting any metadata related to a canister,
CanisterInfo. It takes a config and a name. This should facilitate
working with paths and IDs in general, inside the CLI.

Fixes SDK-607

hansl · 2019-11-08T23:32:11Z

@dfinity-lab/sdk ping

This includes install which does not take a wasm path anymore. We have a new structure for getting any metadata related to a canister, CanisterInfo. It takes a config and a name. This should facilitate working with paths and IDs in general, inside the CLI.

eftychis · 2019-11-11T19:56:27Z

dfx/src/lib/message.rs

@@ -56,7 +50,7 @@ impl UserMessage {

            // dfx build


Can we remove this category then? create a misc one.

paulyoung · 2019-11-11T20:00:38Z

SDK-607 feat: replace using IDs on the command line with project name

I'm guessing this probably uses canister names rather project name

paulyoung · 2019-11-11T20:04:13Z

dfx/src/commands/build.rs

            std::fs::write(&output_wasm_path, wasm)?;

+            // Write the CID.
+            canister_info.create_canister_id()?;


Could we rename the function rather than rely on a comment? I haven't looked yet but I assume that create_canister_id mutates canister_info or writes to dfx.json or both.

It writes to build/<project_name>/canister.id. I'll refactor that.

paulyoung · 2019-11-11T20:16:00Z

dfx/src/commands/canister/install.rs

+    match runtime.block_on(request_status) {
+        Ok(ReadResponse::Pending) => {
+            eprintln!("Pending");
+            println!("0x{}", String::from(request_id));


Can we do something like this?

eprint!("Request ID: "); println!("0x{}", String::from(request_id));

I'm hoping this would read Request ID: 0x...

Same with other places we print the ID.

Sure. Note that this code hasn't been changed (copy-pasted) and we should most definitely rewrite it all when we move the client API to the http lib.

Question: Should we pipe things to different stdout and stderr? Why don't we use the same (stderr)?

STDOUT is scriptable. STDERR is human readable. This is a simplification, but in essence that's what it is. Like in our scripts we use STDOUT of a command to use request-status.

Thanks for the clarification!

paulyoung · 2019-11-11T20:20:56Z

dfx/src/commands/canister/install.rs

-                    print_idl_blob(&blob)
-                        .map_err(|e| DfxError::InvalidData(format!("Invalid IDL blob: {}", e)))?;
+        // Install all canisters.
+        if let Some(canisters) = &config.get_config().canisters {


I think we should consider showing an error saying that a canister name or an --all flag is required. Installing all canisters when a name is omitted seems like an easy way to overwrite canisters by accident.

eftychis · 2019-11-11T20:41:30Z

e2e/build.bash

+
+@test "build outputs the canister ID" {
+    assert_command dfx build
+    [[ -f canisters/hello/_canister.id ]]


Post this PR: Perhaps we should have a random name project test.

We will change the project name soon, so let's do that then.

eftychis · 2019-11-11T22:39:47Z

dfx/src/commands/canister/query.rs

+
+    let canister_name = args.value_of("canister_name").unwrap();
+    let canister_info = CanisterInfo::load(config, canister_name)?;
+    let canister_id = canister_info.get_canister_id().ok_or_else(|| {


Also was done in #147 😁

👍

eftychis · 2019-11-12T00:10:50Z

dfx/src/commands/build.rs

+                canister_info.get_canister_id_path(),
+                canister_info.generate_canister_id()?.into_blob().0,
+            )
+            .map_err(DfxError::from)?;


eftychis · 2019-11-12T00:17:39Z

dfx/src/lib/canister_info.rs

+        // Generate a random u64.
+        let time_since_the_epoch = SystemTime::now()
+            .duration_since(UNIX_EPOCH)
+            .expect("Time went backwards.");


😁 It can happen actually.

Cannot happen with UNIX_EPOCH. Only if I take now()

eftychis · 2019-11-12T00:23:15Z

dfx/src/lib/canister_info.rs

+            .duration_since(UNIX_EPOCH)
+            .expect("Time went backwards.");
+        let cid = u64::from(time_since_the_epoch.as_millis() as u32).shl(32)
+            + u64::from(thread_rng().gen::<u32>());


Why do we need the time here actually?

https://docs.rs/rand/0.6.1/rand/rngs/struct.ThreadRng.html

so we get a u64?

Because we want a monolithically increasing number that is different for each run, but in case 2 runs happen in the same millisecond we want different IDs (hence the randomness).

paulyoung · 2019-11-12T00:34:56Z

dfx/src/lib/message.rs

@@ -66,6 +67,9 @@ impl UserMessage {
            UserMessage::StartNode => "Starts the local network client.",
            UserMessage::NodeAddress => "Specifies the host name and port number to bind the frontend to.",
            UserMessage::StartBackground => "Exits the dfx leaving the client running. Will wait until the client replies before exiting.",
+
+            // misc
+            UserMessage::CanisterName => "Specifies the canister name. If you don't specify this argument, all canisters are processed.",


@hansl looks like this needs updating because of the change to require a canister name.

paulyoung · 2019-11-12T00:36:18Z

dfx/src/commands/canister/install.rs

+        .arg(
+            Arg::with_name("all")
+                .long("all")
+                .required_unless("canister_name")


Do we need a similar change for the canister_name argument to say it's required unless the all flag is passed?

Apparently it doesn't matter.

The message to the user is the same. ¯_(ツ)_/¯

…#145) * feat: replace using IDs on the command line with project name This includes install which does not take a wasm path anymore. We have a new structure for getting any metadata related to a canister, CanisterInfo. It takes a config and a name. This should facilitate working with paths and IDs in general, inside the CLI. * fixup, including fixing bash using stderr and stdout for request id * fixup: add --all * fixup

hansl requested a review from a team as a code owner November 7, 2019 03:40

hansl force-pushed the hansl/canister-names branch from 815a2d8 to 62000ff Compare November 7, 2019 03:47

hansl mentioned this pull request Nov 8, 2019

Remove unwraps (for commands) #147

Merged

hansl force-pushed the hansl/canister-names branch from f32cc03 to 5c8c301 Compare November 9, 2019 20:17

hansl added the automerge-squash label Nov 9, 2019

hansl mentioned this pull request Nov 10, 2019

SDK-544 feat: add support for argument to dfx build #158

Merged

hansl changed the title ~~feat: replace using IDs on the command line with project name~~ SDK-503 feat: replace using IDs on the command line with project name Nov 10, 2019

hansl changed the title ~~SDK-503 feat: replace using IDs on the command line with project name~~ SDK-607 feat: replace using IDs on the command line with project name Nov 10, 2019

eftychis reviewed Nov 11, 2019

View reviewed changes

dfx/src/lib/message.rs

@@ -56,7 +50,7 @@ impl UserMessage {

// dfx build

Copy link

Contributor

eftychis Nov 11, 2019

Choose a reason for hiding this comment

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

Can we remove this category then? create a misc one.

paulyoung suggested changes Nov 11, 2019

View reviewed changes

eftychis reviewed Nov 11, 2019

View reviewed changes

Hans Larsen added 3 commits November 11, 2019 14:49

fixup, including fixing bash using stderr and stdout for request id

cc219e6

fixup: add --all

e808e14

fixup: merge master

0ccd739

eftychis reviewed Nov 12, 2019

View reviewed changes

hansl requested a review from paulyoung November 12, 2019 00:14

eftychis reviewed Nov 12, 2019

View reviewed changes

eftychis approved these changes Nov 12, 2019

View reviewed changes

paulyoung reviewed Nov 12, 2019

View reviewed changes

Hans Larsen added 2 commits November 11, 2019 16:48

fixup

e11d868

Merge branch 'master' into hansl/canister-names

8f3bbe5

paulyoung approved these changes Nov 12, 2019

View reviewed changes

mergify bot merged commit 931f65f into master Nov 12, 2019

mergify bot deleted the hansl/canister-names branch November 12, 2019 01:09

mergify bot removed the automerge-squash label Nov 12, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

SDK-607 feat: replace using IDs on the command line with project name #145

SDK-607 feat: replace using IDs on the command line with project name #145

hansl commented Nov 7, 2019 •

edited

Loading

hansl commented Nov 8, 2019

eftychis Nov 11, 2019

paulyoung commented Nov 11, 2019

paulyoung Nov 11, 2019

hansl Nov 11, 2019

paulyoung Nov 11, 2019

paulyoung Nov 11, 2019

hansl Nov 11, 2019

eftychis Nov 11, 2019

hansl Nov 11, 2019

eftychis Nov 12, 2019

paulyoung Nov 11, 2019

hansl Nov 11, 2019

eftychis Nov 11, 2019

hansl Nov 11, 2019

eftychis Nov 11, 2019

eftychis Nov 12, 2019

eftychis Nov 12, 2019

hansl Nov 12, 2019

eftychis Nov 12, 2019

eftychis Nov 12, 2019

hansl Nov 12, 2019

paulyoung Nov 12, 2019

hansl Nov 12, 2019

paulyoung Nov 12, 2019

hansl Nov 12, 2019

paulyoung Nov 12, 2019

hansl Nov 12, 2019

SDK-607 feat: replace using IDs on the command line with project name #145

SDK-607 feat: replace using IDs on the command line with project name #145

Conversation

hansl commented Nov 7, 2019 • edited Loading

hansl commented Nov 8, 2019

Choose a reason for hiding this comment

paulyoung commented Nov 11, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hansl commented Nov 7, 2019 •

edited

Loading