Skip to content
This repository has been archived by the owner on Sep 26, 2019. It is now read-only.

[PAN-2316] check permissions CLI dependencies #909

Merged
merged 10 commits into from
Feb 19, 2019
Merged

[PAN-2316] check permissions CLI dependencies #909

merged 10 commits into from
Feb 19, 2019

Conversation

macfarla
Copy link
Contributor

PR description

if permissions config file is set, check that either nodes or accounts whitelisting is enabled.

Fixed Issue(s)

fixes #PAN-2316

@@ -552,6 +552,13 @@ public void run() {
+ "or specify the beneficiary of mining (via --miner-coinbase <Address>)");
}

if (permissionsConfigFile != null) {
if (!permissionsAccountsEnabled && !permissionsNodesEnabled) {
throw new ParameterException(
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we want this to be an error? Convention here seems to be options that aren’t going to have an effect is a warning like on line 636.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

have posed the question for @jakehaugen

Copy link
Contributor Author

Choose a reason for hiding this comment

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

changed to warning

Copy link
Contributor

@lucassaldanha lucassaldanha left a comment

Choose a reason for hiding this comment

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

LGTM

@macfarla macfarla merged commit 4668c3b into PegaSysEng:master Feb 19, 2019
@macfarla macfarla deleted the pan-2316-permissions-cli-dependencies branch February 19, 2019 23:38
rain-on pushed a commit to rain-on/pantheon that referenced this pull request Feb 20, 2019
* check nodes or accounts permissioning is enabled if config file is set
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants