-
Notifications
You must be signed in to change notification settings - Fork 211
PHPC-1805 and PHPC-1910: Implement ServerDescription class and Server::getServerDescription() #1239
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
Conversation
5bb2146 to
a8ec9e7
Compare
e5cf895 to
12c64f1
Compare
The GitHub CI test failure for the PHP 8.1 build environment (https://github.com/mongodb/mongo-php-driver/pull/1239/checks?check_run_id=3183644508) is expected I believe - see https://jira.mongodb.org/browse/PHPC-1849
c2c41ed to
2ee222d
Compare
2ee222d to
676f5ca
Compare
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.
Some comments on the code. I'll get to the tests afterwards when those points are addressed and I run this locally.
90b3ad4 to
d7bde2f
Compare
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.
Should be good to go after this round of feedback is addressed.
src/MongoDB/ServerDescription.c
Outdated
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.
For consistency we should declare all variables at the top of the scope. Perhaps you don't see this using clang on macOS, but I get the following error compiling with GCC:
src/MongoDB/ServerDescription.c: In function ‘zim_ServerDescription_getHelloResponse’:
src/MongoDB/ServerDescription.c:44:2: error: ISO C90 forbids mixed declarations and code [-Werror=declaration-after-statement]
Note that newer C standards would permit this (see: https://stackoverflow.com/q/8474100).
I'm not sure why this wasn't caught by CI. Looking at the build status for the PR, only AppVeyor (Windows) appears to have run on the most recent iteration.
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.
IIRC, this is only needed if the test is going to read/write data. Since you're just selecting a server here, we don't need to concern ourselves with the collection under test being empty.
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.
Ok, the same SKIPIF is included in server-debug.phpt - should I make a ticket to remove that?
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.
You can just edit the file in GitHub and generate a PR (instead of directly committing). I don't think we need to track that with a ticket.
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.
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.
Same comment as #1242 (comment)
I was only suggesting that skip_if_not_clean be removed.
b886770 to
1f85854
Compare
I'm not sure why this wasn't caught by CI. Looking at the build status for the PR, only AppVeyor (Windows) appears to have run on the most recent iteration.
I think this is due to the GitHub CI config only expecting certain branches for PRs (see: tests.yml). I just addressed that in 69cc2b5 and merged it into the feature/sdam-monitoring branch. Please rebase and force-push this again to try and trigger a CI build.
The Evergreen config likely has something similar, but it's less flexible as I think we'd need an entirely different project created just for the feature branch. We can do without that for now.
1f85854 to
d01c69b
Compare
I rebased on the feature/sdam-monitoring branch but I think the GitHub CI is still missing the Coding Standards test.
I rebased on the feature/sdam-monitoring branch but I think the GitHub CI is still missing the Coding Standards test.
I didn't realize we had multiple YML configs. Fixed in f3bf5a3 and just merged into the feature branch.
Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>
Co-authored-by: Andreas Braun <alcaeus@users.noreply.github.com>
This partially reverts commit 27dde83 due to build failures.
Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
d01c69b to
966dd92
Compare
The PR description had a reference to PHPC-1849, which I think was incorrect. I changed that to PHPC-1910.
Also, one of the commits says "Implement ServerDescription class and add to monitoring events". Since this PR didn't actually integrate anything with the APM events please make sure that's removed from the message when merging. In fact, I'm not sure any of the individual commit messages need to be kept when squashing, since they don't have any relevance beyond the code review. The PR title sufficiently describes everything in this case.
...::getServerDescription() (mongodb#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
...::getServerDescription() (mongodb#1239) Co-authored-by: Andreas Braun <git@alcaeus.org> Co-authored-by: Jeremy Mikola <jmikola@gmail.com>
Uh oh!
There was an error while loading. Please reload this page.
https://jira.mongodb.org/browse/PHPC-1805
https://jira.mongodb.org/browse/PHPC-1910