-
Notifications
You must be signed in to change notification settings - Fork 33
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
fix: RRSet save comments #144
Conversation
|
||
self::assertSame('www.unit.test.', $result->getName()); | ||
self::assertSame('A', $result->getType()); | ||
self::assertSame(1337, $result->getTtl()); | ||
self::assertCount(1, $result->getRecords()); | ||
self::assertSame('127.0.0.1', $result->getRecords()[0]->getContent()); | ||
self::assertSame('Hello World', $result->getComments()[0]->getContent()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self::assertSame('Hello World', $result->getComments()[0]->getContent()); | |
self::assertSame('Hello World', $result->getComments()[0]->getContent()); | |
self::assertSame('Tester', $result->getComments()[0]->getAccount()); |
@@ -40,6 +53,7 @@ public function testWithArray(): void | |||
self::assertCount(2, $result->getRecords()); | |||
self::assertSame('127.0.0.1', $result->getRecords()[0]->getContent()); | |||
self::assertSame('127.0.0.2', $result->getRecords()[1]->getContent()); | |||
self::assertSame(111, $result->getComments()[1]->getModifiedAt()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
self::assertSame(111, $result->getComments()[1]->getModifiedAt()); | |
self::assertSame(111, $result->getComments()[1]->getModifiedAt()); | |
self::assertSame('World', $result->getComments()[1]->getContent()); | |
self::assertSame('rooti', $result->getComments()[1]->getAccount()); |
Thanks for your PR! It's looking good, but I've made some small suggestions in your tests because you forgot to add some assertions. Also, please run php-cs-fixer to fix this files. Because this is a PR the required GitHub action can't commit to your fork. |
Relevant files are updated 👍 My auto-formatter wants to format differently so no saving + formatting for me :) |
On another note: I don't think it's very friendly to have the syntax for the |
ping ping @trizz :) |
@bennetgallein i've created a discussion for your suggestion about the syntax, can you maybe provide a bit of pseudo code there of what you want to achieve? :) |
Published as v4.5.0! 👍🏻 Thanks! |
Description
Hello, while testing i realized that the code-side of updating/adding comments is there, just missing in the transformers for the actual request. This PR adds the functionality and should work as expected.
Relevant docs: https://doc.powerdns.com/authoritative/http-api/zone.html#rrset
Motivation and context
Well
How has this been tested?
./run-test script runs through no problem, changing my local development env to my github repo and adding/updating comments updates them accordingly
Include details of your testing environment, and the tests you ran to
see how your change affects other areas of the code, etc.
Screenshots (if appropriate)
Checklist:
Go over all the following points, and put an
x
in all the boxes that apply.Please, please, please, don't send your pull request until all of the boxes are ticked. Once your pull request is created, it will trigger a build on our continuous integration server to make sure your tests pass.
If you're unsure about any of these, don't hesitate to ask. We're here to help!