Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cli: add embedded node config #3477

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

AliceInHunterland
Copy link
Contributor

@AliceInHunterland AliceInHunterland commented Jun 4, 2024

If config-path is not provided then the default config is used. If config-path is provided but the directory does not contain a configuration file for the desired passed network and this directory isn't our default ./config, the error "config '%s' doesn't exist" will be received.

Close #3450

@AliceInHunterland AliceInHunterland changed the title vm: add default config cli vm: add default config Jun 4, 2024
Copy link

codecov bot commented Jun 4, 2024

Codecov Report

Attention: Patch coverage is 64.28571% with 10 lines in your changes missing coverage. Please review.

Project coverage is 86.11%. Comparing base (ce1edda) to head (43ee1b2).

Files Patch % Lines
pkg/config/config.go 62.96% 9 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3477      +/-   ##
==========================================
- Coverage   86.11%   86.11%   -0.01%     
==========================================
  Files         331      331              
  Lines       38541    38556      +15     
==========================================
+ Hits        33190    33201      +11     
- Misses       3819     3824       +5     
+ Partials     1532     1531       -1     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@AliceInHunterland AliceInHunterland changed the title cli vm: add default config cli: add default config Jun 5, 2024
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/options/config.go Outdated Show resolved Hide resolved
cli/options/config.go Outdated Show resolved Hide resolved
cli/server/server.go Outdated Show resolved Hide resolved
@AnnaShaleva AnnaShaleva changed the title cli: add default config cli: add embadded node config Jun 5, 2024
@AnnaShaleva
Copy link
Member

What is the default config?

Default config for VM CLI is privnet + InMemory DB.

@AliceInHunterland AliceInHunterland force-pushed the vm-default-config branch 2 times, most recently from bdee5e4 to 319faeb Compare June 5, 2024 15:34
.github/workflows/tests.yml Outdated Show resolved Hide resolved
cli/app/app.go Outdated Show resolved Hide resolved
cli/app/app.go Outdated Show resolved Hide resolved
cli/config/config.go Outdated Show resolved Hide resolved
cli/config/config.go Outdated Show resolved Hide resolved
cli/config/vm.go Outdated Show resolved Hide resolved
cli/config/vm.go Outdated Show resolved Hide resolved
cli/config/vm.go Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
cli/options/options.go Outdated Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the vm-default-config branch 4 times, most recently from 22ad6a6 to bf02042 Compare June 6, 2024 11:13
@AliceInHunterland AliceInHunterland force-pushed the vm-default-config branch 4 times, most recently from 6128144 to 950f35d Compare June 7, 2024 08:00
cli/flags/fixed8_test.go Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
cli/smartcontract/smart_contract.go Outdated Show resolved Hide resolved
cli/config/mainnet.go Outdated Show resolved Hide resolved
cli/config/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/vm/vm.go Outdated Show resolved Hide resolved
cli/flags/address_test.go Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
.github/workflows/tests.yml Outdated Show resolved Hide resolved
cli/options/options.go Outdated Show resolved Hide resolved
cli/options/options.go Outdated Show resolved Hide resolved
config/config_embed.go Outdated Show resolved Hide resolved
config/config_embed.go Show resolved Hide resolved
cli/options/options.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

Linter and tests are failing.

@AnnaShaleva AnnaShaleva changed the title cli: add embadded node config cli: add embedded node config Jun 26, 2024
config/config_embed.go Outdated Show resolved Hide resolved
@AliceInHunterland AliceInHunterland force-pushed the vm-default-config branch 3 times, most recently from de919b6 to cbc1314 Compare June 26, 2024 20:01
// UnitTestnetConfig is the unit testnet configuration.
//
//go:embed protocol.unit_testnet.yml
var UnitTestnetConfig []byte
Copy link
Member

Choose a reason for hiding this comment

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

Do we need this one, btw?

Copy link
Member

Choose a reason for hiding this comment

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

We don't.

Copy link
Contributor Author

@AliceInHunterland AliceInHunterland Jun 27, 2024

Choose a reason for hiding this comment

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

Removed, but we have it as a hidden flag in CLI.
cli.BoolFlag{Name: "unittest", Hidden: true},

config/config_embed.go Outdated Show resolved Hide resolved
// UnitTestnetConfig is the unit testnet configuration.
//
//go:embed protocol.unit_testnet.yml
var UnitTestnetConfig []byte
Copy link
Member

Choose a reason for hiding this comment

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

We don't.

pkg/config/config.go Outdated Show resolved Hide resolved
cli/server/cli_dump_test.go Outdated Show resolved Hide resolved
cli/server/cli_server_test.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
config/config_embed.go Outdated Show resolved Hide resolved
@AnnaShaleva
Copy link
Member

AnnaShaleva commented Jun 27, 2024

Please, fix the commit message: If config-path is not passed....

Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Before re-requesting review, please, perform self-review and ensure that Linter and testing GA jobs pass.

config/config_embed.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
config/config_embed.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
pkg/config/config_test.go Outdated Show resolved Hide resolved
pkg/config/config_test.go Outdated Show resolved Hide resolved
If `config-path` is not passed, default configs are used according to
the set network. In VM CLI the default privnet config with InMemory db
is used.

Close #3450

Signed-off-by: Ekaterina Pavlova <ekt@morphbits.io>
Copy link
Member

@AnnaShaleva AnnaShaleva left a comment

Choose a reason for hiding this comment

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

Please, add "Close #3459" to the commit/PR message.

Otherwise LGTM.

@AnnaShaleva AnnaShaleva mentioned this pull request Jun 28, 2024
3 tasks
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.

Supply VM console with default config
3 participants