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

dnsdist: refactor sendfromto #11651

Merged
merged 1 commit into from
Jun 17, 2022
Merged

dnsdist: refactor sendfromto #11651

merged 1 commit into from
Jun 17, 2022

Conversation

Y7n05h
Copy link
Contributor

@Y7n05h Y7n05h commented May 26, 2022

Signed-off-by: Y7n05h Y7n05h@protonmail.com

Short description

refactor sendfromto

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)

@rgacogne rgacogne self-requested a review May 31, 2022 07:35
omoerbeek
omoerbeek reviewed May 31, 2022
View reviewed changes
Copy link
Member

@omoerbeek omoerbeek left a comment

Choose a reason for hiding this comment

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

As sendfromto() is only used by dnsdist would it be better to move it to dnsdist specific code?

@rgacogne
Copy link
Member

As sendfromto() is only used by dnsdist would it be better to move it to dnsdist specific code?

Sure, we can move it to dnsdist.cc for now, and make it static in the process.

@rgacogne
Copy link
Member

The change looks good to me, thanks!

rgacogne
rgacogne approved these changes May 31, 2022
View reviewed changes
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, thanks!

@rgacogne rgacogne added this to the dnsdist-1.8.0 milestone May 31, 2022
@rgacogne
Copy link
Member

I would like to merge this pull request, but I would prefer if the CI tests could pass before doing so. The error is related to an issue that has been fixed on master, would you mind rebasing this branch on the current master?

@Y7n05h
dnsdist: refactor sendfromto
82e28d6 
Signed-off-by: Y7n05h <Y7n05h@protonmail.com>
@Y7n05h
Copy link
Contributor Author

Y7n05h commented Jun 17, 2022

I would like to merge this pull request, but I would prefer if the CI tests could pass before doing so. The error is related to an issue that has been fixed on master, would you mind rebasing this branch on the current master?

Sure. Now it looks like all the tests have passed.

@rgacogne rgacogne merged commit 2b26d06 into PowerDNS:master Jun 17, 2022
@rgacogne
Copy link
Member

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@omoerbeek omoerbeek omoerbeek left review comments

@rgacogne rgacogne rgacogne approved these changes

Assignees
No one assigned
Labels
dnsdist enhancement
Projects
None yet
Milestone
dnsdist-1.8.0
Development

Successfully merging this pull request may close these issues.
3 participants
@Y7n05h @rgacogne @omoerbeek