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

Remove the externed StatBag from ws-auth #13613

Merged
merged 9 commits into from Dec 22, 2023
Merged

Remove the externed StatBag from ws-auth #13613

merged 9 commits into from Dec 22, 2023

Conversation

fredmorcos
Copy link
Contributor

@fredmorcos fredmorcos commented Dec 13, 2023 •
edited

Short description

This removes the extern StatBag object from ws-auth and instead passes it around as a function/method argument. We should do this, in general, as part of a wider cleanup.

This also cleans up and formats some files automatically using clang-format.

Better reviewed on a per-commit basis.

Checklist

I have:

  • read the CONTRIBUTING.md document
  • compiled this code
  • tested this code
  • included documentation (including possible behaviour changes)
  • documented the code
  • added or modified regression test(s)
  • added or modified unit test(s)

@fredmorcos fredmorcos self-assigned this Dec 13, 2023
@coveralls
Copy link

coveralls commented Dec 13, 2023 •
edited

Pull Request Test Coverage Report for Build 7289561803

  • 763 of 904 (84.4%) changed or added relevant lines in 3 files are covered.
  • 53 unchanged lines in 9 files lost coverage.
  • Overall coverage increased (+1.0%) to 57.682%
Changes Missing Coverage Covered Lines Changed/Added Lines %
pdns/ws-api.cc 132 144 91.67%
pdns/ws-auth.cc 628 757 82.96%
Files with Coverage Reduction New Missed Lines %
ext/luawrapper/include/LuaContext.hpp 1 13.76%
pdns/iputils.cc 2 46.28%
pdns/misc.cc 2 62.51%
modules/gpgsqlbackend/spgsql.cc 3 67.94%
pdns/ws-api.cc 4 85.42%
pdns/dnsdist-carbon.cc 6 59.34%
pdns/dnsdist-tcp.cc 8 75.84%
pdns/recursordist/test-syncres_cc1.cc 13 88.91%
pdns/ws-auth.cc 14 80.52%
Totals Coverage Status
Change from base Build 7287932141: 1.0%
Covered Lines: 107641
Relevant Lines: 155278
💛 - Coveralls

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2023
View reviewed changes
pdns/ws-api.cc Fixed Show fixed Hide fixed
github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 13, 2023
View reviewed changes
pdns/ws-api.cc Fixed Show fixed Hide fixed
@Habbie
Copy link
Member

Habbie commented Dec 14, 2023

This may require a rebase after #13153 which I hope to merge soon.

@Habbie
Copy link
Member

Habbie commented Dec 19, 2023

This indeed requires a rebase now

@Habbie Habbie mentioned this pull request Dec 19, 2023
Closed
7 tasks
@fredmorcos
Copy link
Contributor Author

Rebased and cleaned up ws-auth.cc and ws-api.cc

github-advanced-security[bot]
github-advanced-security bot found potential problems Dec 21, 2023
View reviewed changes
pdns/ws-api.cc
});
}
resp->setJsonBody(doc);
}

void apiServerStatistics(HttpRequest* req, HttpResponse* resp) {
void apiServerStatistics(HttpRequest* req, HttpResponse* resp)

Check warning

Code scanning / CodeQL

Poorly documented large function Warning

Poorly documented function: fewer than 2% comments for a function of 130 lines.
@fredmorcos fredmorcos mentioned this pull request Dec 21, 2023
Merged
7 tasks
chbruyand
chbruyand approved these changes Dec 22, 2023
View reviewed changes
Copy link
Member

@chbruyand chbruyand left a comment

Choose a reason for hiding this comment

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

LGTM!

@fredmorcos fredmorcos merged commit aef27f8 into PowerDNS:master Dec 22, 2023
75 checks passed
@fredmorcos fredmorcos deleted the ws-auth-statbag-cleanup branch December 22, 2023 10:43
Habbie
Habbie reviewed Dec 22, 2023
View reviewed changes
Copy link
Member

@Habbie Habbie left a comment

Choose a reason for hiding this comment

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

I was mid-review when you merged (which is fine!). Submitting my one comment anyway :)

pdns/ws-auth.cc Show resolved Hide resolved
@rgacogne rgacogne mentioned this pull request Jan 9, 2024
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Habbie Habbie Habbie left review comments

@chbruyand chbruyand chbruyand approved these changes

@rgacogne rgacogne Awaiting requested review from rgacogne

@omoerbeek omoerbeek Awaiting requested review from omoerbeek

Assignees

@fredmorcos fredmorcos

Labels
auth
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fredmorcos @coveralls @Habbie @chbruyand