-
Notifications
You must be signed in to change notification settings - Fork 249
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
Add Support for Certificate Mappings #482
Add Support for Certificate Mappings #482
Conversation
Follows the example of other child classes
Also throws an exception when trying to update an invalid field.
Modifications to PHP Opencloud
…into certificate_mappings
…loud into certificate_mappings
{ | ||
$this->assertEquals( | ||
'https://dfw.loadbalancers.api.rackspacecloud.com/v1.0/123456/loadbalancers/2000/ssltermination/certificatemappings', | ||
(string)$this->loadBalancer->CertificateMappings()->Url() |
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.
s/CertificateMappings/certificateMapping
* to a corresponding hostname, allowing multiple SSL certificates to | ||
* exist and be accurately utilized from a Load Balancer. | ||
*/ | ||
class CertificateMappings extends PersistentResource |
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.
Could we rename this to CertificateMapping
since the class represents a singular entity?
@tylerturk @JPry This is awesome, thank you for your hard work 👍 I've left some annotations - but mostly it relates to very small issues. |
Updates based on Rackspace feedback
@jamiehannaford Our pleasure! @ttaylor-wpe was also helpful in this process as he helped to get the ball rolling. We've addressed all the feedback aside from the unit test question. We can definitely add an additional test to validate a response is received, but I agree that it doesn't really test anything useful. Let us know if you need anything else! |
Add Support for Certificate Mappings
Looks great! 🚀 Thanks again @tylerturk @JPry |
@jamiehannaford Thanks for merging this! Do you know when it will move in to production? |
@tylerturk It shipped with v1.12.0 which has just been released 👍 |
Hi @tylerturk, @JPry and @ttaylor-wpe, we would love to send you a little token of appreciation for your contribution to the SDK :) Could you please email your mailing address(es) and t-shirt sizes to us at sdk-support@rackspace.com? Thanks! |
@ycombinator I sent an email. Thanks! |
Summary
The PHP SDK for the Rackspace API lacked support for the new certificate mappings feature recently released.
Approach
A couple of folks (@JPry and @ttaylor-wpe) and myself worked together to add support to the SDK.
Risk
Low
It has been tested and introduces new functionality. We have seen no negative side effects with these code modifications.
Tests
New unit tests have been added to verify that the URL being hit for the API calls matches what is expected.