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

Remove references and logic of system property besu.docker #808

Merged
merged 11 commits into from
May 4, 2020
Merged

Remove references and logic of system property besu.docker #808

merged 11 commits into from
May 4, 2020

Conversation

usmansaleem
Copy link
Member

@usmansaleem usmansaleem commented Apr 29, 2020

PR description

BesuCommand class (and related unit tests) has quite a bit of deprecated logic which sets various defaults depending on if system property besu.docker is present or not. However, this property is not set in Besu's docker file anymore. Hence, the relevant code needs to be removed.

Fixed Issue(s)

#785

Signed-off-by: Usman Saleem <usman@usmans.info>

Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
Signed-off-by: Usman Saleem <usman@usmans.info>
@usmansaleem usmansaleem requested review from jframe and rain-on April 29, 2020 01:31
…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
@usmansaleem usmansaleem requested a review from shemnon April 29, 2020 20:29
…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
Copy link
Contributor

@jframe jframe left a comment

Choose a reason for hiding this comment

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

LGTM

@@ -1960,42 +1954,19 @@ private String genesisConfig() {
}

private File genesisFile() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Doesn't seem to be much benefit in wrapping this in a function

} else {
return getDefaultBesuDataPath(this);
}
return standaloneCommands.dataPath.toAbsolutePath();
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as above, think we can inline this function where it is needed

@@ -991,9 +986,7 @@ void setBesuConfiguration(final BesuConfiguration pluginCommonConfiguration) {

private BesuCommand handleStandaloneCommand() {
standaloneCommands = new StandaloneCommand();
Copy link
Contributor

Choose a reason for hiding this comment

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

Is folding StandaloneCommand back into Besu Command part of this PR or a follow on PR?

Copy link
Member Author

Choose a reason for hiding this comment

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

I missed that bit, I've added it into same PR.

…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
…k into BesuCommand

Signed-off-by: Usman Saleem <usman@usmans.info>
…perty

Signed-off-by: Usman Saleem <usman@usmans.info>
@usmansaleem usmansaleem merged commit e8bd7ad into hyperledger:master May 4, 2020
@usmansaleem usmansaleem deleted the docker_system_property branch May 4, 2020 23:50
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.

3 participants