feat: add config command #50

hansl · 2019-10-01T18:23:09Z

This will make e2e tests more robust and stable, and is very useful
in general.

dfx/src/commands/config.rs

paulyoung

Some e2e tests would be good

paulyoung · 2019-10-01T22:41:51Z

dfx/src/commands/config.rs

+    SubCommand::with_name("config")
+        .about("Configure options in the current DFINITY project.")
+        .arg(
+            Arg::with_name("config_path")


Does "path" imply a jq-like path, as in .defaults.start.port? What about arrays?

I think a couple of examples would be good to put in the help.

It's JSON-Pointers. I'll come up with tests and examples.

paulyoung · 2019-10-01T22:44:30Z

dfx/src/commands/config.rs

+
+pub fn exec<T>(_env: &T, args: &ArgMatches<'_>) -> DfxResult {
+    // Cannot use the `env` variable as we need a mutable copy.
+    let mut config = Config::from_current_dir()?;


I'm not sure it really matters in this case but can we say something like <T: Clone> and then use that to get a mutable copy?

Config is a large structure to clone. I dunno. I guess? Stream of thoughts here is that there's only one place where the config should be mutable and it's in this function. The rest of the CLI should only read it.

Another option would be to have a get_mut_config() on the environment. I'm more okay with that one, but it's a bit more involved in the typing system (have to change both implementation of the trait and internal types).

Config is a large structure to clone. I dunno. I guess?

FWIW, I vote that all structures without unbounded-sized binary blobs inside are fine to clone. Structures with such blobs can also be cloned if they wrap those blobs with defensive Rc<_> wrappers. We shouldn't be afraid to .clone().

Done. Please take another look.

hansl · 2019-10-01T23:00:52Z

I agree with e2e, waiting for my other PR to make it in.

hansl · 2019-10-04T20:37:50Z

@matthewhammer @paulyoung Ready for a second round of review.

matthewhammer

LGTM

The notes I give about bash are not critical, and shouldn't block the PR.

matthewhammer · 2019-10-07T08:31:57Z

e2e/config.bash

+
+    run dfx config defaults/build/output
+    [[ $status == 0 ]]
+    [[ "$output" == "\"build/\"" ]]


I don't know bash well.
Is this pattern (repeated below) something that cannot be made into a subroutine in bash?

Bash is touted as universal and readable, but this script has lots of "advanced" stuff, degrading its role as a readable testing script.

Is it possible to use subroutines with readable names for these more cryptic bash idioms?

Yes we probably should, but I'm not sure which part you want to use as a subroutine. Lets sync up offline.

eftychis · 2019-10-07T18:06:33Z

dfx/src/commands/config.rs

@@ -0,0 +1,56 @@
+use crate::lib::env::ProjectConfigEnv;
+use crate::lib::error::{DfxError, DfxResult};
+use clap::{App, Arg, ArgMatches, SubCommand};


eftychis · 2019-10-07T18:09:03Z

dfx/src/commands/config.rs

+    // Cannot use the `env` variable as we need a mutable copy.
+    let mut config = env
+        .get_config()
+        .ok_or(DfxError::CommandMustBeRunInAProject())?


Maybe ok_or_else which is lazily evaluating the argument?

eftychis · 2019-10-07T18:13:07Z

dfx/src/commands/config.rs

+        .ok_or(DfxError::CommandMustBeRunInAProject())?
+        .clone();
+
+    let config_path = args.value_of("config_path").unwrap();


Handle error?

Although I could, those arguments are already validated by clap and if they're missing clap would already have errored out.

Hmm, I see. Weird. Let me take another quick look.

I see it is required (required=true).

If any of the required arguments are refactored to become optional then this would crash at runtime, and the compiler won't help point that out.

That's true, but there's nothing we can do outside of testing all combinations; returning a "helpful" error message is not better than a crash.

I have a JIRA ticket to write better errors messages and remove a bunch of unwrap. That could be part of this.

eftychis · 2019-10-07T18:14:40Z

dfx/src/commands/config.rs

+        let value = serde_json::from_str::<Value>(arg_value)
+            .unwrap_or_else(|_| Value::String(arg_value.to_owned()));
+
+        // Try to parse the type of the 5293value (which is a string). By default we will just assume


fixed and clarified.

eftychis · 2019-10-07T18:16:03Z

dfx/src/commands/config.rs

+
+        config.save()
+    } else if let Some(value) = config.get_json().pointer(config_path.as_str()) {
+        println!("{}", value);


Why are we printing the value of the path? Is it the best approach or for sanity?

dfx config a/b/c should print the value at path /a/b/c. That's how script (and sometimes users) should read the config.

hansl

PTAL.

eftychis

LGTM

This will make e2e tests more robust and stable, and is very useful in general.

@hansl

commit 7c5e71488df430b89de637a15b56814db196e752 Author: Islam El-Ashi <ielashi@users.noreply.github.com> Date: Sat Nov 7 00:30:05 2020 +0100 feat: DER-encode public key and principal ID when using Plain Authentication. (#48) BREAKING CHANGE If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed. commit a254861ae4bfe608734c8c190e4033cfe39f047b Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:32 2020 -0800 chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 694849109bc28fea80119388504127e889b36d15 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:14 2020 -0800 chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:16:53 2020 -0800 chore(deps-dev): bump node-fetch in /packages/bootstrap (#51) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit ba4b082e5795150176f7adad989c3613edceb85f Author: Hans <hans@larsen.online> Date: Thu Nov 5 12:49:59 2020 -0800 test: add ic-ref, ci e2e and lerna support to repo (#47) This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection). commit 8baa1cfc227d003c2806df1057b92009159df250 Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:36 2020 -0800 feat: use anonymous principal by default in HttpAgent (#46) If the auth transform is missing and the principal is anonymous, use an anonymous auth transform. Otherwise, use the auth transform as normal. # BREAKING CHANGE Before, an auth transform would not be called if it was missing, now it results as an error if the principal is not anonymous. This was because just using the packet as is without putting it in an envelope was a bug and should never be sent to a replica anyway. Now this is explicit. commit 1a714e6c53f5e467112b964b71204b6d2eb545ef Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:15 2020 -0800 feat: add a getPrincipal() method to Agents (#40) commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4 Author: Hans <hans@larsen.online> Date: Tue Nov 3 08:49:44 2020 -0800 feat: add the canister ID to the window.ic object (#41) This can be useful to know the canister ID of the current frontend. commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919 Author: Hans <hans@larsen.online> Date: Mon Nov 2 21:57:21 2020 -0800 feat: add a cache bust hash at the end of javascript output (#39) commit 9f2ed968b7cc9f4b854af5e23edca19d34124909 Author: Benjamin Goering <benjamin.goering@dfinity.org> Date: Tue Sep 22 15:28:49 2020 -0700 docs: polish docs/npm-publish.md per @hansl feedback (#37) via: pr feedback on dfinity/agent-js#36

@hansl

commit 19c28008db5fa1582da573088b17d0bdee9e8bc2 Author: Hans <hans@larsen.online> Date: Mon Nov 9 14:47:25 2020 -0800 revert: Remove lerna (PR #47) (#59) This removes lerna, but attempt to not remove the e2e tests and the new packages/files that were added after the last PR. commit 426a94f356bc4e27f50c8bec55071b82105adbd1 Author: Hans <hans@larsen.online> Date: Mon Nov 9 13:54:04 2020 -0800 feat: use anonymous for the first retrieve (#53) Also include some cleanup of the scripts that arent used, and add a diagram for the bootstrapping process. This does not affect the worker yet. Only the first retrieve of the index.js on localhost. BREAKING CHANGE If a canister has a retrieve() method that relies on Principal, the principal used will change. Co-authored-by: Benjamin Goering <benjamin.goering@dfinity.org> commit b795d8b34ce38dd35e1bcf2b80e33108cf74161d Author: Andrew Wylde <2126677+codelemur@users.noreply.github.com> Date: Mon Nov 9 09:41:13 2020 -0800 feat(idp): add identity-provider package to repository (#52) adds plain HTML/ts and test setup for project commit 7c5e71488df430b89de637a15b56814db196e752 Author: Islam El-Ashi <ielashi@users.noreply.github.com> Date: Sat Nov 7 00:30:05 2020 +0100 feat: DER-encode public key and principal ID when using Plain Authentication. (#48) BREAKING CHANGE If a developer relies on the inner work of SenderPubKey or manually instanciate Principal, the API changed. commit a254861ae4bfe608734c8c190e4033cfe39f047b Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:32 2020 -0800 chore(deps): bump node-fetch from 2.6.0 to 2.6.1 in /e2e/node (#49) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 694849109bc28fea80119388504127e889b36d15 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:17:14 2020 -0800 chore(deps-dev): bump node-fetch from 2.6.0 to 2.6.1 in /packages/agent (#50) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit 88af47f79b5e9f3a5e08abbf0ce03b3140357fc0 Author: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> Date: Fri Nov 6 15:16:53 2020 -0800 chore(deps-dev): bump node-fetch in /packages/bootstrap (#51) Bumps [node-fetch](https://github.com/bitinn/node-fetch) from 2.6.0 to 2.6.1. - [Release notes](https://github.com/bitinn/node-fetch/releases) - [Changelog](https://github.com/node-fetch/node-fetch/blob/master/docs/CHANGELOG.md) - [Commits](node-fetch/node-fetch@v2.6.0...v2.6.1) Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> commit ba4b082e5795150176f7adad989c3613edceb85f Author: Hans <hans@larsen.online> Date: Thu Nov 5 12:49:59 2020 -0800 test: add ic-ref, ci e2e and lerna support to repo (#47) This refactors the repository to use lerna to manage its monorepo's packages. It also adds the e2e tests from the SDK repo that we previously had to remove. More e2e tests will be added later, but this unblocks working with DER and Identity work as we can now at least run some e2e tests now (instead of mocking the http connection). commit 8baa1cfc227d003c2806df1057b92009159df250 Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:36 2020 -0800 feat: use anonymous principal by default in HttpAgent (#46) If the auth transform is missing and the principal is anonymous, use an anonymous auth transform. Otherwise, use the auth transform as normal. # BREAKING CHANGE Before, an auth transform would not be called if it was missing, now it results as an error if the principal is not anonymous. This was because just using the packet as is without putting it in an envelope was a bug and should never be sent to a replica anyway. Now this is explicit. commit 1a714e6c53f5e467112b964b71204b6d2eb545ef Author: Hans <hans@larsen.online> Date: Tue Nov 3 11:04:15 2020 -0800 feat: add a getPrincipal() method to Agents (#40) commit 324e856944adb7f79a2afa2cffbdb6d144b35ab4 Author: Hans <hans@larsen.online> Date: Tue Nov 3 08:49:44 2020 -0800 feat: add the canister ID to the window.ic object (#41) This can be useful to know the canister ID of the current frontend. commit 8680a2cf4994b45c6e00ffb0b5a12b379f5ff919 Author: Hans <hans@larsen.online> Date: Mon Nov 2 21:57:21 2020 -0800 feat: add a cache bust hash at the end of javascript output (#39) commit 9f2ed968b7cc9f4b854af5e23edca19d34124909 Author: Benjamin Goering <benjamin.goering@dfinity.org> Date: Tue Sep 22 15:28:49 2020 -0700 docs: polish docs/npm-publish.md per @hansl feedback (#37) via: pr feedback on dfinity/agent-js#36

…5000 (#50) Co-authored-by: smallstepman <21069150+smallstepman@users.noreply.github.com>

hansl requested a review from a team as a code owner October 1, 2019 18:23

matthewhammer suggested changes Oct 1, 2019

View reviewed changes

dfx/src/commands/config.rs Outdated Show resolved Hide resolved

dfx/src/commands/config.rs Outdated Show resolved Hide resolved

paulyoung suggested changes Oct 1, 2019

View reviewed changes

hansl requested review from paulyoung and matthewhammer October 1, 2019 23:02

hansl force-pushed the hansl/config branch from 43f816b to e8bb9ce Compare October 4, 2019 20:37

hansl requested a review from a team October 5, 2019 00:00

matthewhammer approved these changes Oct 7, 2019

View reviewed changes

eftychis reviewed Oct 7, 2019

View reviewed changes

hansl commented Oct 7, 2019

View reviewed changes

hansl requested a review from eftychis October 7, 2019 20:17

eftychis approved these changes Oct 7, 2019

View reviewed changes

Hans Larsen added 3 commits October 7, 2019 14:51

feat: add config command

493527f

This will make e2e tests more robust and stable, and is very useful in general.

refactor: allow Clone on config

baefc31

test: e2e tests for dfx config

ae4ed81

hansl force-pushed the hansl/config branch from 0074b13 to ae4ed81 Compare October 7, 2019 21:52

hansl merged commit 7e364af into master Oct 7, 2019

hansl deleted the hansl/config branch October 7, 2019 22:15

smallstepman added a commit that referenced this pull request Apr 6, 2023

chore: update replica version to 742d0a8399476a83878bb87c74eb46b2d8e0…

277307b

…5000 (#50) Co-authored-by: smallstepman <21069150+smallstepman@users.noreply.github.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add config command #50

feat: add config command #50

hansl commented Oct 1, 2019

paulyoung left a comment

paulyoung Oct 1, 2019

hansl Oct 1, 2019

paulyoung Oct 1, 2019

hansl Oct 1, 2019

matthewhammer Oct 1, 2019

hansl Oct 4, 2019

hansl commented Oct 1, 2019

hansl commented Oct 4, 2019

matthewhammer left a comment

matthewhammer Oct 7, 2019

matthewhammer Oct 7, 2019

hansl Oct 7, 2019

eftychis Oct 7, 2019

eftychis Oct 7, 2019

hansl Oct 7, 2019

eftychis Oct 7, 2019

hansl Oct 7, 2019

eftychis Oct 7, 2019

eftychis Oct 7, 2019

paulyoung Oct 7, 2019

hansl Oct 7, 2019

eftychis Oct 7, 2019

hansl Oct 7, 2019

eftychis Oct 7, 2019

hansl Oct 7, 2019

hansl left a comment

eftychis left a comment

feat: add config command #50

feat: add config command #50

Conversation

hansl commented Oct 1, 2019

paulyoung left a 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 Oct 1, 2019

hansl commented Oct 4, 2019

matthewhammer left a 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 left a comment

Choose a reason for hiding this comment

eftychis left a comment

Choose a reason for hiding this comment