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

feat(zebra-network): add user agent argument #6601

Merged
merged 6 commits into from
May 5, 2023
Merged

feat(zebra-network): add user agent argument #6601

merged 6 commits into from
May 5, 2023

Conversation

oxarbitrage
Copy link
Contributor

@oxarbitrage oxarbitrage commented May 3, 2023

Motivation

We want to give zebra-network the possibility of changing the user agent it will use and we also want Zebra to automatically build the user agent by using git data.

Close #2375

Solution

  • From zebrad, build and pass valid user agent to network.
  • Build the user agent too in zebra-rpc as it is needed in the getinfo rpc method. We have app_version data already here, was needed for the same getinfo function, so we have everything to build user agent there too.

Review

I think anyone can review, this reduce the number of steps from the release checklist.

Reviewer Checklist

  • Will the PR name make sense to users?
    • Does it need extra CHANGELOG info? (new features, breaking changes, large changes)
  • Are the PR labels correct?
  • Does the code do what the ticket and PR says?
    • Does it change concurrent code, unsafe code, or consensus rules?
  • How do you know it works? Does it have tests?

Follow Up Work

@github-actions github-actions bot added C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG labels May 3, 2023
@teor2345
Copy link
Contributor

teor2345 commented May 3, 2023

I just want to say that I love deleting stuff from the release checklist 🎉

@codecov
Copy link

codecov bot commented May 3, 2023

Codecov Report

Merging #6601 (44a38d1) into main (58bd898) will increase coverage by 0.09%.
The diff coverage is 71.05%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #6601      +/-   ##
==========================================
+ Coverage   77.82%   77.92%   +0.09%     
==========================================
  Files         308      309       +1     
  Lines       40331    40653     +322     
==========================================
+ Hits        31386    31677     +291     
- Misses       8945     8976      +31     

@oxarbitrage oxarbitrage self-assigned this May 3, 2023
@oxarbitrage oxarbitrage added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code P-Medium ⚡ A-compatibility Area: Compatibility with other nodes or wallets, or standard rules labels May 3, 2023
@oxarbitrage oxarbitrage marked this pull request as ready for review May 3, 2023 19:49
@oxarbitrage oxarbitrage requested review from a team as code owners May 3, 2023 19:49
@oxarbitrage oxarbitrage requested review from upbqdn and removed request for a team May 3, 2023 19:49
@teor2345 teor2345 removed the request for review from a team May 3, 2023 20:49
@teor2345
Copy link
Contributor

teor2345 commented May 4, 2023

Just letting you know that PR #6606 changes the format of the git version string in some cases. I don't know if that will impact this PR.

If you'd like, you can add:

Depends-On: #6601

to the PR #6606 description, which will make my PR wait for this one to merge. Then if anything breaks I can fix it.

(Just so you know, I'm not planning on reviewing this PR, because Marek is assigned to it.)

upbqdn
upbqdn previously approved these changes May 4, 2023
Copy link
Member

@upbqdn upbqdn left a comment

Choose a reason for hiding this comment

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

Looks great. I left two optional suggestions and one question.

zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
zebra-rpc/src/methods.rs Outdated Show resolved Hide resolved
Co-authored-by: Marek <mail@marek.onl>
@oxarbitrage
Copy link
Contributor Author

Thanks! I added your doc suggestions and i tried to reply the question.

@oxarbitrage oxarbitrage requested a review from upbqdn May 4, 2023 19:39
mergify bot added a commit that referenced this pull request May 4, 2023
mergify bot added a commit that referenced this pull request May 4, 2023
@mergify mergify bot merged commit 7c67512 into main May 5, 2023
@mergify mergify bot deleted the issue2375 branch May 5, 2023 00:29
@oxarbitrage oxarbitrage mentioned this pull request May 9, 2023
38 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-compatibility Area: Compatibility with other nodes or wallets, or standard rules A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-feature Category: New features C-trivial Category: A trivial change that is not worth mentioning in the CHANGELOG
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Allow different zebra-network apps to use different user agents
3 participants