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

Move snapsync-bft-enabled to stable options #7930

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

pullurib
Copy link
Contributor

PR description

Fixed Issue(s)

#7924

Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

needs a changelog entry - this will be a breaking change

@macfarla macfarla added the doc-change-required Indicates an issue or PR that requires doc to be updated label Nov 27, 2024
Copy link
Contributor

@macfarla macfarla left a comment

Choose a reason for hiding this comment

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

you'll also want to remove
hidden = true,
so the option shows in the help

Copy link
Contributor

@siladu siladu left a comment

Choose a reason for hiding this comment

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

IMO better to avoid the breaking change and make this an alias and call out the deprecation of the X version, e.g. see

// TODO --Xbonsai-limit-trie-logs-enabled and --Xbonsai-trie-log-pruning-enabled are deprecated,
// remove in a future release
@SuppressWarnings("ExperimentalCliOptionMustBeCorrectlyDisplayed")
@Option(
names = {
LIMIT_TRIE_LOGS_ENABLED,
"--Xbonsai-limit-trie-logs-enabled", // deprecated
"--Xbonsai-trie-log-pruning-enabled" // deprecated
},
fallbackValue = "true",
description = "Limit the number of trie logs that are retained. (default: ${DEFAULT-VALUE})")
private Boolean limitTrieLogsEnabled = DEFAULT_LIMIT_TRIE_LOGS_ENABLED;

Then can add entry to changelog under "Upcoming Breaking Changes"

@pullurib
Copy link
Contributor Author

Thanks for the suggestions. So currently I will

  • add comments in the code to indicate deprecation in future release
  • Add changelog entry for upcoming breaking changes
  • Update documentation to indicate this option deprecation soon and that it will be enabled by default then

@matthew1001
Copy link
Contributor

Yes I think I we should remove the option, not promote it to non-experimental.

So I can see the case for making this PR a changelog entry to mark it deprecated, followed by a PR after the next release or two have gone out, which basically reverses the PR I originally added the new flag under.

@macfarla macfarla marked this pull request as draft November 27, 2024 22:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
doc-change-required Indicates an issue or PR that requires doc to be updated
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants