WIP check api #592
Draft
WIP check api #592
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
matthewhilton
commented
Jan 15, 2024
| @@ -89,25 +91,40 @@ public function generate_presigned_url($contenthash, $headers = array()) { | |||
| */ | |||
| public function define_client_check() { | |||
| global $OUTPUT; | |||
|
|
|||
| // TODO use MDL check api admin setting if available ??? | |||
There was a problem hiding this comment.
TODO use https://tracker.moodle.org/browse/MDL-67898 if available, since it uses Ajax so page loading times are very quick
classes/local/store/s3/client.php
Outdated
Comment on lines
302
to
304
| $summarystr = $status == result::OK ? 'check:passed' : 'check:failed'; | ||
| $summary = get_string($summarystr, 'tool_objectfs'); | ||
| return new result($status, $summary, $details); |
There was a problem hiding this comment.
Per above, just return an OK result here and remove the variables
classes/local/store/s3/client.php
Outdated
Comment on lines
325
to
369
| $this->client->putObject([ | ||
| 'Bucket' => $this->bucket, | ||
| 'Key' => $this->bucketkeyprefix . 'permissions_check_file', | ||
| 'Body' => 'test content')); | ||
| 'Body' => 'test content', | ||
| ]); | ||
| } catch (\Aws\S3\Exception\S3Exception $e) { | ||
| $details = $this->get_exception_details($e); | ||
| $permissions->messages[get_string('settings:writefailure', 'tool_objectfs') . $details] = 'notifyproblem'; | ||
| $permissions->success = false; | ||
| $exdetails = $this->get_exception_details($e); | ||
| $details .= get_string('settings:writefailure', 'tool_objectfs') . $exdetails; | ||
| $status = result::ERROR; | ||
| } | ||
|
|
||
| // Try to get an object. | ||
| try { | ||
| $result = $this->client->getObject(array( | ||
| $this->client->getObject([ | ||
| 'Bucket' => $this->bucket, | ||
| 'Key' => $this->bucketkeyprefix . 'permissions_check_file')); | ||
| 'Key' => $this->bucketkeyprefix . 'permissions_check_file', | ||
| ]); | ||
| } catch (\Aws\S3\Exception\S3Exception $e) { | ||
| $errorcode = $e->getAwsErrorCode(); | ||
| // Write could have failed. | ||
| if ($errorcode !== 'NoSuchKey') { | ||
| $details = $this->get_exception_details($e); | ||
| $permissions->messages[get_string('settings:permissionreadfailure', 'tool_objectfs') . $details] = 'notifyproblem'; | ||
| $permissions->success = false; | ||
| $exdetails = $this->get_exception_details($e); | ||
| $details .= get_string('settings:permissionreadfailure', 'tool_objectfs') . $exdetails; | ||
| $status = result::ERROR; | ||
| } | ||
| } | ||
|
|
||
| // Try to delete an object. | ||
| if ($testdelete) { | ||
| try { | ||
| $result = $this->client->deleteObject(array('Bucket' => $this->bucket, 'Key' => $this->bucketkeyprefix . 'permissions_check_file')); | ||
| $permissions->messages[get_string('settings:deletesuccess', 'tool_objectfs')] = 'warning'; | ||
| $permissions->success = false; | ||
| $this->client->deleteObject([ | ||
| 'Bucket' => $this->bucket, | ||
| 'Key' => $this->bucketkeyprefix . 'permissions_check_file', | ||
| ]); | ||
| $details .= get_string('settings:deletesuccess', 'tool_objectfs'); | ||
| } catch (\Aws\S3\Exception\S3Exception $e) { | ||
| $errorcode = $e->getAwsErrorCode(); | ||
| // Something else went wrong. | ||
| if ($errorcode !== 'AccessDenied') { | ||
| $details = $this->get_exception_details($e); | ||
| $permissions->messages[get_string('settings:deleteerror', 'tool_objectfs') . $details] = 'notifyproblem'; | ||
| $permissions->success = false; | ||
| $details .= get_string('settings:deleteerror', 'tool_objectfs') . $details; | ||
| $status = result::ERROR; | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Ideally revert this back to previous and just implode the array of messages at the end - a bit simpler and less diff
classes/local/store/swift/client.php
Outdated
| } | ||
| } | ||
|
|
||
| return $connection; | ||
| return new result(result::OK, get_string('check:success', 'tool_objectfs')); |
There was a problem hiding this comment.
Lang string should be check:passed not check:success
| } | ||
|
|
||
| public function test_permissions($testdelete) { | ||
| public function test_permissions($testdelete): result { |
There was a problem hiding this comment.
PHP doc required
| new tool_objectfs\check\store(\tool_objectfs\check\store::TYPE_PERMISSIONS), | ||
| new tool_objectfs\check\store(\tool_objectfs\check\store::TYPE_CONNECTION), | ||
| ]; | ||
|
|
||
| if (get_config('tool_objectfs', 'proxyrangerequests')) { |
There was a problem hiding this comment.
Should we check signing support here as well ? That's checked inside the proxy_range_request check
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
A work in progress update to add some basic checks to objectfs - related to #480
Basically the idea is to expose the current methods we have available in
object_clientvia the check api https://moodledev.io/docs/apis/subsystems/checktest_permissionstest_connectiontest_range_requestAs well as add a new method:
4.
test_configurationto detect any config level problems e.g. missing keys, missing config, etc...1,2,3 are basically the same code, just calling a different method, so these share a check API class. 3. is only enabled if range requests are enabled.
4 is special since it runs only if the
$CFG->alternative_file_systemis set, whereas 1,2,3 only run if the configuration is OK (i.e. 4 is passing)Note this only targets
MOODLE_310_STABLEsince lower versions (< 3.9) do not have the check API.TODO
test_*functions - return check results.test_*methods to try to avoid any regressions with the function signature change.get_client_checksand make them all use the check API