-
Notifications
You must be signed in to change notification settings - Fork 149
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 Sahara data processing support #157
base: master
Are you sure you want to change the base?
Conversation
@ekasitk Is this PR ready for review? Thanks |
@haphan Yes, it is ready for review. |
Can you rebase with |
@haphan Rebase was done. Could you review it again? |
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.
Hi @ekasitk, very high quality PR. Just a few comments below; mainly to follow existing standard and convention of other services in this SDK
- Add description and return types where possible (
src/DataProcessing/v1/Service.php
). This will help to generate very nice reference doc. - Add unit tests
- Nice to have: integration tests.
'neutronManagementNetwork' => '{neutronManagementNetworkId}', | ||
]; | ||
|
||
$cluster = $sahara->createCluster($options); |
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.
Add type hint to $cluster
e.g. /**@var OpenStack\DataProcessing\v1\Models\Cluster $cluster */
|
||
$clusters = $sahara->createCluster($options); | ||
foreach ($clusters as $cluster) { | ||
print_r($cluster); |
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.
Add type hint
|
||
$sahara = $openstack->dataProcessingV1(['region' => '{region}']); | ||
|
||
$cluster = $sahara->getCluster(['id' => '{clusterId}']); |
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.
Add type hint
|
||
$sahara = $openstack->dataProcessingV1(['region' => '{region}']); | ||
|
||
$cluster = $sahara->getCluster(['id' => '{clusterId}']); |
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.
Add type hint
]; | ||
|
||
$clusters = $sahara->listClusters($options); | ||
foreach ($clusters as $cluster) { |
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.
Add type hint to $client
|
||
$sahara = $openstack->dataProcessingV1(['region' => '{region}']); | ||
|
||
$cluster = $sahara->getCluster(['id' => '{clusterId}']); |
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.
Add type hint
'name' => '{ClusterTemplateName}', | ||
]; | ||
|
||
$clusterTemplate = $sahara->createClusterTemplate($options); |
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.
Add type hint
protected $resourcesKey = 'images'; | ||
|
||
protected $aliases = [ | ||
'OS-EXT-IMG-SIZE:size' => 'OSEXTIMGSIZEsize', |
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.
Can we simplify to size
?
This PR provides the first version of sahara data processing support (ref. https://developer.openstack.org/api-ref/data-processing/).