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

Updating confusing --bonsai-maximum-back-layers-to-load option to --bonsai-historical-block-limit + remove deprecated snapshots config #5337

Merged
merged 11 commits into from
Apr 18, 2023

Conversation

non-fungible-nelson
Copy link
Contributor

PR description

Changed the mentioned flags. Alias used to keep backwards compatibility. Also removed now deprecated snapshots option.

Fixed Issue(s)

Ambiguity

@non-fungible-nelson non-fungible-nelson added the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 12, 2023
@github-actions
Copy link

github-actions bot commented Apr 12, 2023

  • I thought about documentation and added the doc-change-required label to this PR if updates are required.
  • I have considered running ./gradlew acceptanceTestNonMainnet locally if my PR affects non-mainnet modules.
  • I thought about the changelog and included a changelog update if required.

CHANGELOG.md Outdated Show resolved Hide resolved
@non-fungible-nelson non-fungible-nelson changed the title Updating confusing --bonsai-maximum-back-layers-to-load option to --bonsai-historical-block-limit Updating confusing --bonsai-maximum-back-layers-to-load option to --bonsai-historical-block-limit + remove deprecated snapshots config Apr 13, 2023
@non-fungible-nelson
Copy link
Contributor Author

gotta love more red than green on a PR

@macfarla
Copy link
Contributor

there is one unit test failing

java.lang.AssertionError: 
Expecting actual:
  "Invalid value for option '--bonsai-maximum-back-layers-to-load': 'ten' is not a long

To display full help:
besu [COMMAND] --help
"
to contain:
  "Invalid value for option '--bonsai-historical-block-limit': 'ten' is not a long" 
	at org.hyperledger.besu.cli.BesuCommandTest.parsesInvalidBonsaiTrieBonsaiHistoricalBlockLimit(BesuCommandTest.java:1912)

…aStorageOptions.java

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Nelson <[email protected]>
@macfarla
Copy link
Contributor

there is one unit test failing

@non-fungible-nelson I believe PicoCLI is choosing the longest name to throw into that error message.

You could look at how host-whitelist is handled - it's a hidden alternative (separate option) to host-allowlist - but then you have to handle the case where both are supplied

also if you don't hide that option name it will still display in the help ->

      --bonsai-historical-block-limit,
        --bonsai-maximum-back-layers-to-load=<LONG>
                             Limit of historical layers that can be loaded with
                               BONSAI (default: 512).

Alternatively - if we don't mind users still seeing the old name, you could just remove that test - it's not critical

@non-fungible-nelson
Copy link
Contributor Author

I think seeing both is OK if you're querying for help directly. As long as we are clear in the docs and it supplies to the client correctly.

@macfarla macfarla enabled auto-merge (squash) April 18, 2023 18:32
@macfarla macfarla merged commit 560c548 into hyperledger:main Apr 18, 2023
@non-fungible-nelson non-fungible-nelson deleted the bonsai-naming-change branch April 18, 2023 20:32
@bgravenorst bgravenorst removed the doc-change-required Indicates an issue or PR that requires doc to be updated label Apr 18, 2023
elenduuche pushed a commit to elenduuche/besu that referenced this pull request Aug 16, 2023
…onsai-historical-block-limit + remove deprecated snapshots config (hyperledger#5337)

* Changing bonsai-max-back-layers-to-load to --bonsai-historical-block-limit

Signed-off-by: Matt Nelson <[email protected]>

* remove a couple other places where snapshot option was configured

Signed-off-by: garyschulte <[email protected]>

* Update besu/src/main/java/org/hyperledger/besu/cli/options/stable/DataStorageOptions.java

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Nelson <[email protected]>

---------

Signed-off-by: Matt Nelson <[email protected]>
Signed-off-by: Matt Nelson <[email protected]>
Signed-off-by: garyschulte <[email protected]>
Co-authored-by: garyschulte <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
…onsai-historical-block-limit + remove deprecated snapshots config (hyperledger#5337)

* Changing bonsai-max-back-layers-to-load to --bonsai-historical-block-limit

Signed-off-by: Matt Nelson <[email protected]>

* remove a couple other places where snapshot option was configured

Signed-off-by: garyschulte <[email protected]>

* Update besu/src/main/java/org/hyperledger/besu/cli/options/stable/DataStorageOptions.java

Co-authored-by: Sally MacFarlane <[email protected]>
Signed-off-by: Matt Nelson <[email protected]>

---------

Signed-off-by: Matt Nelson <[email protected]>
Signed-off-by: Matt Nelson <[email protected]>
Signed-off-by: garyschulte <[email protected]>
Co-authored-by: garyschulte <[email protected]>
Co-authored-by: Sally MacFarlane <[email protected]>
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.

4 participants