SDK-607 feat: replace using IDs on the command line with project name by hansl · Pull Request #145 · dfinity/sdk

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

            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

Comments

Conversation

hansl commented Nov 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hansl commented Nov 8, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

paulyoung commented Nov 11, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hansl commented Nov 7, 2019 •

edited

Loading