Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Remove old deprecated flags, move light client, updater and private tx flags to deprecated section #11694

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

Conversation

vorot93
Copy link

@vorot93 vorot93 commented May 9, 2020

This PR removes pre-2.7 deprecated flags, and moves light client, updater and private tx toggles to their place instead. Also sets updater filter to Never by default and fixes parity mentions in a couple of places.

Also see #11681 and #11682

@vorot93 vorot93 added A0-pleasereview 🤓 Pull request needs code review. B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. labels May 9, 2020
@vorot93 vorot93 force-pushed the vorot93/cli-update branch 3 times, most recently from 72f0724 to 525fea6 Compare May 10, 2020 20:43
@vorot93 vorot93 removed the B9-blocker 🚧 This pull request blocks the next release from happening. Use only in extreme cases. label May 11, 2020
parity/deprecated.rs Outdated Show resolved Hide resolved
parity/deprecated.rs Outdated Show resolved Hide resolved
parity/deprecated.rs Outdated Show resolved Hide resolved
parity/deprecated.rs Outdated Show resolved Hide resolved
ARG arg_rpcport: (Option<u16>) = None, or |_| None,
"--rpcport=[PORT]",
"Equivalent to --jsonrpc-port PORT.",
FLAG flag_no_hardcoded_sync: (bool) = false, or |c: &Config| c.parity.as_ref()?.no_hardcoded_sync,
Copy link
Collaborator

@niklasad1 niklasad1 May 11, 2020

Choose a reason for hiding this comment

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

This is light client-related also, should probably have the same comment as --light so it's not forgotten if/when the light client is removed.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a sub command that is used to produce the hard coded headers too? Needs deprecating too then?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Do you mean export-hardcoded-sync? Then yes

ARG arg_rpcapi: (Option<String>) = None, or |_| None,
"--rpcapi=[APIS]",
"Equivalent to --jsonrpc-apis APIS.",
FLAG flag_no_serve_light: (bool) = false, or |c: &Config| c.network.as_ref()?.no_serve_light.clone(),
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same here, see https://github.com/openethereum/openethereum/pull/11694/files#r423164093

I assume the full nodes won't implement PIP after #11681, thus this flag won't do anything

@niklasad1 niklasad1 added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label May 11, 2020
@@ -1204,17 +986,6 @@ struct PrivateTransactions {
#[serde(deny_unknown_fields)]
struct Ui {
Copy link
Collaborator

Choose a reason for hiding this comment

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

can this be removed?

Copy link
Author

Choose a reason for hiding this comment

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

I believe this is used by signer

Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate? If it is, and Signer is going to be around still, maybe it's good to add a comment here to that effect?

parity/main.rs Outdated Show resolved Hide resolved
parity/deprecated.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

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

Looks good overall but these changes need to be clearly communicated to the users because it might break their configuration and expectations.

}
/// Removed flags will go here.
pub fn find_deprecated(_args: &Args) -> Vec<Deprecated> {
Vec::new()
Copy link
Collaborator

Choose a reason for hiding this comment

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

So after this PR there are no deprecated flags at all?

Deprecated::Removed("--whisper-pool-size"),
]);
}
/// Removed flags will go here.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
/// Removed flags will go here.
/// Returns a list of removed or currently deprecated CLI flags.

@@ -49,7 +49,7 @@ use parity_daemonize::AsHandle;
use parking_lot::{Condvar, Mutex};

const PLEASE_RESTART_EXIT_CODE: i32 = 69;
const PARITY_EXECUTABLE_NAME: &str = "parity";
const CLIENT_EXECUTABLE_NAME: &str = "openethereum";
Copy link
Collaborator

Choose a reason for hiding this comment

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

Maybe just EXECUTABLE_NAME?

ARG arg_rpcport: (Option<u16>) = None, or |_| None,
"--rpcport=[PORT]",
"Equivalent to --jsonrpc-port PORT.",
FLAG flag_no_hardcoded_sync: (bool) = false, or |c: &Config| c.parity.as_ref()?.no_hardcoded_sync,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Isn't there a sub command that is used to produce the hard coded headers too? Needs deprecating too then?

@@ -1204,17 +986,6 @@ struct PrivateTransactions {
#[serde(deny_unknown_fields)]
struct Ui {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can you elaborate? If it is, and Signer is going to be around still, maybe it's good to add a comment here to that effect?

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A0-pleasereview 🤓 Pull request needs code review. B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants