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

auth LUA: make whitespace insertion on chunk combine optional #14021

Merged
merged 1 commit into from May 2, 2024

Conversation

Habbie
Copy link
Member

@Habbie Habbie commented Apr 2, 2024

Short description

draft. Needs docs. clang-tidy will likely complain about short variable names. sorted

On backport to 4.9, make sure to flip the default for this setting to 'yes'

fixes #14002

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)
  • checked that this code was merged to master

pdns/dnsrecords.cc Outdated Show resolved Hide resolved
@coveralls
Copy link

coveralls commented Apr 2, 2024

Pull Request Test Coverage Report for Build 8598988894

Details

  • 2 of 2 (100.0%) changed or added relevant lines in 1 file are covered.
  • 24 unchanged lines in 5 files lost coverage.
  • Overall coverage decreased (-0.003%) to 59.495%

Files with Coverage Reduction New Missed Lines %
pdns/packethandler.cc 1 72.86%
pdns/iputils.cc 2 48.01%
pdns/signingpipe.cc 2 84.41%
pdns/recursordist/test-syncres_cc1.cc 5 89.82%
modules/lmdbbackend/lmdbbackend.cc 14 72.65%
Totals Coverage Status
Change from base Build 8598761943: -0.003%
Covered Lines: 114086
Relevant Lines: 159293

💛 - Coveralls

@Habbie Habbie added this to the auth-5 milestone Apr 4, 2024
@Habbie Habbie marked this pull request as ready for review April 8, 2024 11:03
@neilcook
Copy link
Contributor

I tested this and indeed it works as expected at execution time - a long Lua script that previous failed due to the whitespace issue now runs just fine and returns a result.
However I found that when querying the API for the record, the whitespace still appeared, which I found quite confusing, particularly as I found this before I tried executing the script, so I initially assumed it wasn't working.

@Habbie
Copy link
Member Author

Habbie commented Apr 11, 2024

However I found that when querying the API for the record, the whitespace still appeared, which I found quite confusing, particularly as I found this before I tried executing the script, so I initially assumed it wasn't working.

good one. Same in pdnsutil edit-zone. I wonder if we can do the join there too, but we don't want to confuse people who did manual chunking at the same time.

@Habbie
Copy link
Member Author

Habbie commented Apr 23, 2024

when querying the API for the record, the whitespace still appeared

After merging this PR, I'll reopen the original issue to cover that part of the question. We'll backport only this PR (with the default set to whitespace insertion) to 4.9 and might do API and list-zone/edit-zone for 5.0.

@rgacogne rgacogne self-requested a review May 2, 2024 13:00
Copy link
Member

@rgacogne rgacogne left a comment

Choose a reason for hiding this comment

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

Looks good!

@Habbie Habbie merged commit 964e713 into PowerDNS:master May 2, 2024
76 checks passed
@Habbie Habbie deleted the auth-lua-join-whitespace branch May 2, 2024 19:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

auth API: rechunk LUA records according to settings
4 participants