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

changed user UserAgent name #2956

Closed
wants to merge 6 commits into from
Closed

changed user UserAgent name #2956

wants to merge 6 commits into from
+8 −1

Conversation

ATREAY
Copy link
Contributor

@ATREAY ATREAY commented Nov 24, 2023 •
edited
Loading

Description

This pull request introduces modifications to the user agent/network name.

Issues

fixes: #2932 , #2908

@github-actions github-actions bot added the external Issues created by non node team members label Nov 24, 2023
@Wondertan
Copy link
Member

@ATREAY, This works and solves the issue. Let's also fix #2908 in this PR. You can get it from nodebuilder/node.GetBuildInfo

@ATREAY
Copy link
Contributor Author

ATREAY commented Nov 26, 2023

Certainly, @Wondertan.
Since I'm new to the repo, could you please clarify whether the system should return the semantic version or the system version?

@Wondertan
Copy link
Member

@ATREAY, semantic. System version should stay private

@ATREAY
Copy link
Contributor Author

ATREAY commented Nov 27, 2023

Hey @Wondertan, I included the network version. Could you confirm if this meets the requirements mentioned in the issue?

Wondertan
Wondertan reviewed Nov 27, 2023
View reviewed changes
nodebuilder/p2p/host.go Outdated Show resolved Hide resolved
@ATREAY
Copy link
Contributor Author

ATREAY commented Nov 30, 2023

Hey @Wondertan, I have formatted the user agent name. Could you please check and confirm if it is correct when you have a moment?

Wondertan
Wondertan reviewed Nov 30, 2023
View reviewed changes
nodebuilder/p2p/host.go Outdated Show resolved Hide resolved
@ATREAY ATREAY requested a review from ramin as a code owner December 1, 2023 04:56
@ATREAY
Copy link
Contributor Author

ATREAY commented Dec 1, 2023

Hey @Wondertan, Since params.Net contains comprehensive information about the network name and its alias, I believe this should suffice.

@Wondertan
Copy link
Member

@ATREAY, No. It must be in the following format celestia-node/Arabica-10/v0.12/abc2das, currently it will produce Arabica-10-node/v0.12/abc2das. Again, celestia-node should stay constant.

Also, I made a typo initially, and it should be 7 first chars of commit instead of 8.

Please write a unit test that ensures we follow the requested UserAgent form

@ATREAY
Copy link
Contributor Author

ATREAY commented Dec 2, 2023

Hey @Wondertan

I want to inform you that I lack familiarity with unit testing techniques and creating unit tests, as I have not worked on them previously. If feasible, could you please assign this task to someone with experience in unit testing? This would help prevent unnecessary errors that may arise if I were to undertake this task.

I apologize for any inconvenience this may cause.

@ramin
Copy link
Contributor

ramin commented Dec 6, 2023

@ATREAY i can help with getting some unit tests in place so we can get this merged

@ramin ramin assigned ramin and ATREAY Dec 6, 2023
@ramin ramin added area:config CLI and config area:node Node labels Dec 6, 2023
@ramin
Copy link
Contributor

ramin commented Dec 11, 2023

@ATREAY i created some tests and refactored code here a7c7b16

have a look, if you like and agree i can push the changes to this branch and we can see about getting this merged

@ATREAY
Copy link
Contributor Author

ATREAY commented Dec 12, 2023

@ATREAY i created some tests and refactored code here a7c7b16

have a look, if you like and agree i can push the changes to this branch and we can see about getting this merged

I think it's good. What do you think, @Wondertan? Is it okay?

@ramin ramin added the kind:misc Attached to miscellaneous PRs label Jan 10, 2024
@ramin ramin mentioned this pull request Jan 10, 2024
Closed
@ramin
Copy link
Contributor

ramin commented Mar 8, 2024

@Wondertan check this with some expanded tests, if you like we can replace this PR with it and get over the line: ATREAY#1

@Wondertan
Copy link
Member

@ramin, lets use your tested version! We can open your PR directly to node

@ramin ramin mentioned this pull request May 8, 2024
Merged
@ramin
Copy link
Contributor

ramin commented May 8, 2024

replaced by #3379

@ramin ramin closed this May 8, 2024
ramin added a commit that referenced this pull request May 10, 2024
@ramin @ATREAY
refactor: changed user UserAgent name (#3379)
86f4b2a 
<!--
Thank you for submitting a pull request!

Please make sure you have reviewed our contributors guide before
submitting your
first PR.

Please ensure you've addressed or included references to any related
issues.

Tips:
- Use keywords like "closes" or "fixes" followed by an issue number to
automatically close related issues when the PR is merged (e.g., "closes
#123" or "fixes #123").
- Describe the changes made in the PR.
- Ensure the PR has one of the required tags (kind:fix, kind:misc,
kind:break!, kind:refactor, kind:feat, kind:deps, kind:docs, kind:ci,
kind:chore, kind:testing)

-->

Opening as a replacement for
#2956 which has gone
stale. I had added some tests to the original refactoring but original
author vanished without allowing admin's to modify the original
PR/branch.

From original note:

fixes: #2932 ,
#2908

---------

Co-authored-by: Atreay Kukanur <[email protected]>
@ramin ramin mentioned this pull request Jul 3, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Wondertan Wondertan Wondertan left review comments

@renaynay renaynay Awaiting requested review from renaynay renaynay is a code owner

@vgonkivs vgonkivs Awaiting requested review from vgonkivs vgonkivs is a code owner

@distractedm1nd distractedm1nd Awaiting requested review from distractedm1nd

@walldiss walldiss Awaiting requested review from walldiss walldiss is a code owner

@ramin ramin Awaiting requested review from ramin

Assignees

@ramin ramin

@ATREAY ATREAY

Labels
area:config CLI and config area:node Node external Issues created by non node team members kind:misc Attached to miscellaneous PRs
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

modp2p: change user UserAgent name
3 participants
@ATREAY @Wondertan @ramin