Skip to content

Made MemoryStore the default whithout config.json for neo-cli#3085

Merged
shargon merged 2 commits intoneo-project:masterfrom
cschuchardt88:fix-memorystore-cli
Jan 12, 2024
Merged

Made MemoryStore the default whithout config.json for neo-cli#3085
shargon merged 2 commits intoneo-project:masterfrom
cschuchardt88:fix-memorystore-cli

Conversation

@cschuchardt88
Copy link
Member

@cschuchardt88 cschuchardt88 commented Jan 12, 2024 •
edited
Loading

Description

Made MemoryStore the default for neo-cli settings.

Type of change

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • This change requires a documentation update

How Has This Been Tested?

  • Test config.json

Checklist:

  • My code follows the style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@shargon
Copy link
Member

shargon commented Jan 12, 2024

I think that the default one should be persistent

vncoelho
vncoelho reviewed Jan 12, 2024
View reviewed changes
Copy link
Member

@vncoelho vncoelho left a comment

Choose a reason for hiding this comment

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

Usually we always use a config.json file @shargon
I think that the config default can be kept as LevelDb and this one as MemoryStore

@shargon
Copy link
Member

shargon commented Jan 12, 2024

Usually we always use a config.json file @shargon I think that the config default can be kept as LevelDb and this one as MemoryStore

It sounds reasonable that if it doesn't have config.json it won't be persistent, what do you think @Jim8y ?

@Jim8y
Copy link
Contributor

Jim8y commented Jan 12, 2024 •
edited by vncoelho
Loading

Will it work at all? Cause the node will always have a config that sets leveldb by default. How to make it work? Delete the config file?

@shargon
Copy link
Member

shargon commented Jan 12, 2024

The config.json should overwrite this default, @Jim8y. I still did not test the PR. Maybe other adjustments are needed.

@Jim8y ? xD

@shargon shargon changed the title Made MemoryStore the default for neo-cli Made MemoryStore the default whithout config.json for neo-cli Jan 12, 2024
@Jim8y
Copy link
Contributor

Jim8y commented Jan 12, 2024

The config.json should overwrite this default, @Jim8y. I still did not test the PR. Maybe other adjustments are needed.

What happend to my comments? this is not mine.....@shargon

@Jim8y
Copy link
Contributor

Jim8y commented Jan 12, 2024

I mean, how does the default setting here to be effective? As long as you have a config in the cli, it will overwrite the setting here, so delete the config file?

@vncoelho
Copy link
Member

vncoelho commented Jan 12, 2024

image

My mystake, crazy github!

@vncoelho
Copy link
Member

vncoelho commented Jan 12, 2024

I just reverted the edit

@vncoelho
Copy link
Member

vncoelho commented Jan 12, 2024 •
edited
Loading

image

image

It worked in .devcontainer by just deleting the specification from config.json

@shargon
Copy link
Member

shargon commented Jan 12, 2024

The config.json should overwrite this default, @Jim8y. I still did not test the PR. Maybe other adjustments are needed.

What happend to my comments? this is not mine.....@shargon

I didn't remove anything...

@shargon shargon merged commit a294c9c into neo-project:master Jan 12, 2024
@shargon shargon deleted the fix-memorystore-cli branch January 12, 2024 13:08
@vncoelho
Copy link
Member

vncoelho commented Jan 12, 2024

@Jim8y, maybe we create now a:

  • configSimplified.json with minimal configs (without leveldb), withoutwallet and etc...
  • update Readme with an example about how to add all this on command line

Jim8y added a commit to Jim8y/neo that referenced this pull request Jan 12, 2024
@Jim8y
Merge branch 'master' into fix-build
c426977 
* master:
  Made `MemoryStore` the default whithout `config.json` for `neo-cli` (neo-project#3085)
  Adding Devcontainer and link to codespace (neo-project#3075)
  Update & Consolidate nugets (neo-project#3083)
  Adding NNS to `neo-cli` (neo-project#3032)
  Add: add pull request template (neo-project#3081)
  Move to monorepo: Neo.Cryptography.BLS12_381 (neo-project#3077)
  Add: add a new verify result status code (neo-project#3076)
  Convert to Neo.Json and Neo.ConsoleService to `dotnet` standard 2.1 (neo-project#3044)
  Avoid IsExternalInit  (neo-project#3079)
  Clean usings (neo-project#3078)
  Fixed asp.net core project (neo-project#3067)
  Updated BLS12_381 (neo-project#3074)
  avoid nonsense exception messages. (neo-project#3063)
  Removed `MyGet` (neo-project#3071)
  Updated unit-test (neo-project#3073)
  add hash verification for OnImport (neo-project#3070)
  Make public ReadUserInput (neo-project#3068)
  Removed asp.net core (neo-project#3065)
  Enforce Line Endings in `.editorconfig` (neo-project#3060)

# Conflicts:
#	src/Neo.CLI/CLI/MainService.cs
@cschuchardt88
Copy link
Member Author

cschuchardt88 commented Jan 13, 2024 •
edited
Loading

I think we should have config.json but have the others and just let people pick the one they want.

@roman-khimov roman-khimov added this to the v3.7.0 milestone Feb 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@shargon shargon shargon approved these changes

@Jim8y Jim8y Jim8y approved these changes

@vncoelho vncoelho vncoelho approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

v3.7.0

Development

Successfully merging this pull request may close these issues.

5 participants

@cschuchardt88 @shargon @Jim8y @vncoelho @roman-khimov