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.

Sign up for GitHub

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

Jump to bottom

ETCM-365: Version info in Hello.clientId #794

Merged
merged 12 commits into from
Nov 18, 2020
Merged

ETCM-365: Version info in Hello.clientId #794

merged 12 commits into from
Nov 18, 2020

Conversation

aakoshh
Copy link
Contributor

@aakoshh aakoshh commented Nov 13, 2020 •
edited
Loading

Description

The PR adds the conventional Ethereum node name in the WireProtocol.Hello.clientId field, for example: mantis/v3.0-cd5ae33/linux-amd64/ubuntu-openjdk64bitservervm-java-11.0.9.

The -Dmantis.client-identity property can be used to splice a user identity into it to distinguish our client, perhaps by a ticket number or just belonging to an organisation.

I didn't do as an elaborate normalisation as besu in the PlatformDetector, called from BesuInfo; seemed like an overkill.

@aakoshh aakoshh requested a review from ntallar November 13, 2020 16:51
@aakoshh aakoshh force-pushed the ETCM-365-hello-client-id branch from c281d52 to 5210423 Compare November 13, 2020 16:53
@aakoshh aakoshh force-pushed the ETCM-365-hello-client-id branch from 5210423 to 9684d23 Compare November 13, 2020 16:56
ntallar
ntallar reviewed Nov 13, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I didn't do as an elaborate normalisation as besu in the PlatformDetector, called from BesuInfo; seemed like an overkill.

Agree!

Apart from one comment LGTM!

src/main/resources/application.conf Outdated Show resolved Hide resolved
ntallar
ntallar reviewed Nov 17, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

Apart from a last comment, LGTM!

src/main/resources/application.conf Outdated Show resolved Hide resolved
ntallar
ntallar approved these changes Nov 18, 2020
View reviewed changes
Copy link

@ntallar ntallar left a comment

Choose a reason for hiding this comment

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

LGTM!

 .\\            //.
. \ \          / /.
.\  ,\     /` /,.-
 -.   \  /'/ /  .
 ` -   `-'  \  -
   '.       /.\`
      -    .-
      :`//.'
      .`.'
      .' BP

@ntallar ntallar merged commit 9ba2205 into develop Nov 18, 2020
@aakoshh aakoshh deleted the ETCM-365-hello-client-id branch November 18, 2020 17:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ntallar ntallar ntallar approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@aakoshh @ntallar