[Cli] Add `default_prompt_response` option to `set_global_config` command #5614

fgrust · 2022-11-17T13:19:43Z

[WIP]

Description

Updates for issue #2205

Test Plan

This PR is in WIP, so far saving the configuration in the config file works well.

$ aptos config set-global-config --default-prompt_response yes
$ aptos config show-global-config
{
  "Result": {
    "config_type": "Global",
    "default_prompt_response": "Yes"
  }
}

This change is

gregnazario

Hey this is a great first start!

I think you'll want to do one of these two options:

Make an enum with, [yes, no, prompt], where prompt is default
Take advantage of Option<bool>, to handle None as prompt.

It's probably easier from a readability perspective for the first one, and then it can default to prompt.

Then you can loop this into the PromptOptions to read the global config and use this as the default behavior rather than what it does right now

banool · 2022-11-17T17:55:58Z

Agree with Greg, particularly with the first thing he suggests.

fgrust · 2022-11-18T17:30:36Z

Hey this is a great first start!

I think you'll want to do one of these two options:

Make an enum with, [yes, no, prompt], where prompt is default

Take advantage of Option<bool>, to handle None as prompt.

It's probably easier from a readability perspective for the first one, and then it can default to prompt.

Then you can loop this into the PromptOptions to read the global config and use this as the default behavior rather than what it does right now

I also agree with the first thing.

Please review the updated code, and then I will move forward with PromptOptions as the config logic is approved.

banool · 2022-11-18T18:54:45Z

Will look next week :)

banool · 2022-11-20T16:13:39Z

crates/aptos/src/config/mod.rs

@@ -174,6 +184,9 @@ pub struct GlobalConfig {
    /// Whether to be using Global or Workspace mode
    #[serde(skip_serializing_if = "Option::is_none")]
    pub config_type: Option<ConfigType>,
+    /// Prompt response type
+    #[serde(skip_serializing_if = "Option::is_none")]
+    pub default_prompt_response: Option<PromptResponseType>,


Can we use serde(default) here instead of making this an Option? As it is now the Option doesn't really make much sense given there is also a Prompt variant of the PromptReponseType enum.

In that case, it would save default_prompt_response: Prompt in the config file, even if we run set-global-config without --default-prompt-response option. It's what we should come up with?

banool · 2022-11-20T16:14:31Z

crates/aptos/src/config/mod.rs

+            ASSUME_YES => Ok(Self::Yes),
+            ASSUME_NO => Ok(Self::No),
+            _ => Err(CliError::CommandArgumentError(
+                "Invalid prompt response type, must be one of [yes, no]".to_string(),


It could also be prompt right?

banool · 2022-11-27T20:39:07Z

Awesome work, this looks like it is good to go to me! My only concern now is if this PR lands, users might think the feature is ready, but it doesn't actually affect anything yet. Could you perhaps stack then next PR on top of this one and then we'll land them all together?

fgrust · 2022-11-28T09:13:46Z

Awesome work, this looks like it is good to go to me! My only concern now is if this PR lands, users might think the feature is ready, but it doesn't actually affect anything yet. Could you perhaps stack then next PR on top of this one and then we'll land them all together?

Sure thing!. I am doing the exact implementation. thanks.

fgrust · 2022-11-28T17:53:34Z

I thought prompt_yes_with_override function in common/utils.rs is good place to overwrite prompt_options parameter and run. do you have the better idea? this line

banool · 2022-11-28T20:38:57Z

crates/aptos/src/common/utils.rs

+    }
+
+    if !prompt_options.assume_yes {
+        match GlobalConfig::load()?.get_default_prompt_response() {


A couple of points here:

First, GlobalConfig only reads the config at the global folder (e.g. ~/.aptos), we want to read the correct config based on scope. So if there is an .aptos dir in the current dir, use that. You want something like this instead:

let config = CliConfig::load(ConfigSearchMode::CurrentDirAndParents)?;

Second, we shouldn't load the config here, it should be passed in, since in some cases we might already have it. That way we avoid unnecessary double reads of the config.

First - As default_prompt_response is saved in global config file, I think using GlobalConfig::load rather makes sense. CliConfig::load loads the profiles only. Otherwise, it should be stored in the profiles as needed.
Second - I agree with your principle. GlobalConfig::load is used to get .aptos folder path mostly to load profiles, save them, and check the config existence and called more than once to load and then save in a line. Furthermore, it's not designed to pass the global config from the root to the leaf, which means it should be redesigned and/or needed much updates relatively.

@banool, thoughts?

First: Okay I may be misunderstanding the code, I'd have to dive deeper. Though for now I'll invoke @gregnazario to comment since he knows the CLI better than anyone 😛

Second: Same thing, what do you think @gregnazario? I see instances where we have a config already, it seems redundant to load it again. Perhaps the config object should be smarter and act as a passthrough cache.

Sorry for the delay, been very very busy.

I would put it in the global config like he does here, and I think it's the right choice.

This code maybe should read a slightly different way. This will let you override via CLI flags default behavior even if you sent it, allow you to set a default which overtakes so you don't need to send flags, and allow for default prompting otherwise.

This pseudocode below I think lets it be very simple and straightforward for someone to read afterwards and tell what's going on.

// Handle CLI flags If assume_no: return abort else if assume_yes: return Ok // Then handle default or prompt behavior bool is_yes = if let Some(response) = default_response: response else: prompt and get response if is_yes: return Ok else: return abort

I see now you have Prompt as an option, then replace the end there with a match statement

thanks for the reply. I just did some changes. could you please review again?

github-actions · 2022-12-06T17:34:57Z

✅ Forge suite `land_blocking` success on `f1a570fa1109dcebdc096b0e29a7c8c539fda56c`

performance benchmark with full nodes : 6819 TPS, 5816 ms latency, 9700 ms p99 latency,no expired txns
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

github-actions · 2022-12-06T17:38:36Z

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `f1a570fa1109dcebdc096b0e29a7c8c539fda56c`

Compatibility test results for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f1a570fa1109dcebdc096b0e29a7c8c539fda56c (PR)
1. Check liveness of validators at old version: testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b
compatibility::simple-validator-upgrade::liveness-check : 7144 TPS, 5406 ms latency, 7100 ms p99 latency,no expired txns
2. Upgrading first Validator to new version: f1a570fa1109dcebdc096b0e29a7c8c539fda56c
compatibility::simple-validator-upgrade::single-validator-upgrade : 4826 TPS, 8246 ms latency, 10700 ms p99 latency,no expired txns
3. Upgrading rest of first batch to new version: f1a570fa1109dcebdc096b0e29a7c8c539fda56c
compatibility::simple-validator-upgrade::half-validator-upgrade : 4828 TPS, 8258 ms latency, 10900 ms p99 latency,no expired txns
4. upgrading second batch to new version: f1a570fa1109dcebdc096b0e29a7c8c539fda56c
compatibility::simple-validator-upgrade::rest-validator-upgrade : 6799 TPS, 5650 ms latency, 10000 ms p99 latency,no expired txns
5. check swarm health
Compatibility test for testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f1a570fa1109dcebdc096b0e29a7c8c539fda56c passed
Test Ok

Grafana dashboard
Humio Logs
Test runner output
Test run is land-blocking

…mand (aptos-labs#5614) * [cli] add default_prompt_response option to set_global_config * update options * update global config struct * add prompt response getter * apply default prompt option config * update prompt func * update prompt func * fix lint issue * fix lint error

fgrust requested a review from gregnazario as a code owner November 17, 2022 13:19

fgrust mentioned this pull request Nov 17, 2022

Add aptos config --set do-not-prompt or something similar #2205

Closed

gregnazario requested a review from banool November 17, 2022 17:47

gregnazario reviewed Nov 17, 2022

View reviewed changes

fgrust requested review from gregnazario and removed request for banool November 18, 2022 17:30

fgrust requested review from bchocho, msmouse, lightmark, davidiw, banool, clay-aptos, zekun000 and a team as code owners November 18, 2022 18:34

banool requested changes Nov 20, 2022

View reviewed changes

fgrust requested review from banool and removed request for gregnazario, bchocho, msmouse, lightmark, davidiw, clay-aptos and zekun000 November 23, 2022 15:57

banool requested changes Nov 28, 2022

View reviewed changes

gregnazario enabled auto-merge (squash) December 6, 2022 05:28

This comment has been minimized.

Sign in to view

auto-merge was automatically disabled December 6, 2022 07:13
Head branch was pushed to by a user without write access

banool enabled auto-merge (squash) December 6, 2022 14:40

auto-merge was automatically disabled December 6, 2022 14:57
Head branch was pushed to by a user without write access

fgrust added 9 commits December 6, 2022 09:58

[cli] add default_prompt_response option to set_global_config

d25afa2

update options

d0476de

update global config struct

5a4794c

add prompt response getter

115c0ec

apply default prompt option config

e2ab18e

update prompt func

0078bcc

update prompt func

c179e20

fix lint issue

9d3373e

fix lint error

f1a570f

banool enabled auto-merge (squash) December 6, 2022 14:59

banool disabled auto-merge December 6, 2022 16:53

banool enabled auto-merge (squash) December 6, 2022 16:53

This comment has been minimized.

Sign in to view

banool disabled auto-merge December 6, 2022 18:08

banool merged commit 73c9034 into aptos-labs:main Dec 6, 2022

fgrust deleted the prompt_response_option branch December 7, 2022 21:07

Markuze mentioned this pull request Dec 26, 2022

markuze/aptos perf #5998

Closed

Markuze mentioned this pull request Jan 3, 2023

[Network] Adding AptosPerf Client #6054

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Cli] Add `default_prompt_response` option to `set_global_config` command #5614

[Cli] Add `default_prompt_response` option to `set_global_config` command #5614

fgrust commented Nov 17, 2022 •

edited

Loading

gregnazario left a comment

banool commented Nov 17, 2022 •

edited

Loading

fgrust commented Nov 18, 2022 •

edited

Loading

banool commented Nov 18, 2022

banool Nov 20, 2022

fgrust Nov 23, 2022 •

edited

Loading

banool Nov 20, 2022

banool commented Nov 27, 2022

fgrust commented Nov 28, 2022

fgrust commented Nov 28, 2022 •

edited

Loading

banool Nov 28, 2022

fgrust Nov 28, 2022

banool Nov 28, 2022

gregnazario Dec 2, 2022 •

edited

Loading

fgrust Dec 5, 2022

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 6, 2022

github-actions bot commented Dec 6, 2022

[Cli] Add default_prompt_response option to set_global_config command #5614

[Cli] Add default_prompt_response option to set_global_config command #5614

Conversation

fgrust commented Nov 17, 2022 • edited Loading

Description

Test Plan

gregnazario left a comment

Choose a reason for hiding this comment

banool commented Nov 17, 2022 • edited Loading

fgrust commented Nov 18, 2022 • edited Loading

banool commented Nov 18, 2022

banool Nov 20, 2022

Choose a reason for hiding this comment

fgrust Nov 23, 2022 • edited Loading

Choose a reason for hiding this comment

banool Nov 20, 2022

Choose a reason for hiding this comment

banool commented Nov 27, 2022

fgrust commented Nov 28, 2022

fgrust commented Nov 28, 2022 • edited Loading

banool Nov 28, 2022

Choose a reason for hiding this comment

fgrust Nov 28, 2022

Choose a reason for hiding this comment

banool Nov 28, 2022

Choose a reason for hiding this comment

gregnazario Dec 2, 2022 • edited Loading

Choose a reason for hiding this comment

fgrust Dec 5, 2022

Choose a reason for hiding this comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

github-actions bot commented Dec 6, 2022

✅ Forge suite land_blocking success on f1a570fa1109dcebdc096b0e29a7c8c539fda56c

github-actions bot commented Dec 6, 2022

✅ Forge suite compat success on testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b ==> f1a570fa1109dcebdc096b0e29a7c8c539fda56c

[Cli] Add `default_prompt_response` option to `set_global_config` command #5614

[Cli] Add `default_prompt_response` option to `set_global_config` command #5614

fgrust commented Nov 17, 2022 •

edited

Loading

banool commented Nov 17, 2022 •

edited

Loading

fgrust commented Nov 18, 2022 •

edited

Loading

fgrust Nov 23, 2022 •

edited

Loading

fgrust commented Nov 28, 2022 •

edited

Loading

gregnazario Dec 2, 2022 •

edited

Loading

✅ Forge suite `land_blocking` success on `f1a570fa1109dcebdc096b0e29a7c8c539fda56c`

✅ Forge suite `compat` success on `testnet_2d8b1b57553d869190f61df1aaf7f31a8fc19a7b` ==> `f1a570fa1109dcebdc096b0e29a7c8c539fda56c`