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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 2 commits into from
Jan 12, 2024
Merged

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

merged 2 commits into from
Jan 12, 2024

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

image

My mystake, crazy github!

@vncoelho
Copy link
Member

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
2 checks passed
@shargon shargon deleted the fix-memorystore-cli branch January 12, 2024 13:08
@vncoelho
Copy link
Member

@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.

None yet
5 participants
@cschuchardt88 @shargon @Jim8y @vncoelho @roman-khimov