Skip to content
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 bucket options and key management #66

Open
wants to merge 13 commits into
base: master
Choose a base branch
from

Conversation

macbookandrew
Copy link

@mlambley here’s a followup PR for #56. Thanks!

@macbookandrew
Copy link
Author

I’m not sure what’s up with the failing scrutinizer tests…I’m more familiar with Gitlab CI.

When running phpunit locally, I’m seeing only one failure: the testCopy method fails due to this line passing incorrect parameters to listFiles.

@mlambley
Copy link
Member

mlambley commented Feb 5, 2021

@macbookandrew If you pull from master you'll get the fix I just applied. My apologies for that.

@macbookandrew
Copy link
Author

@mlambley Thanks—updated and I’m seeing all passing tests now.

Copy link
Member

@mlambley mlambley left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm requesting quite a few changes here, let me know if you want me to handle any of them myself. Cheers.

protected $namePrefix;
protected $expirationTimestamp;

/**
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add the new params to this docblock.


class Key
{
const PERMISSION_LIST_KEYS = 'listKeys';
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to include these constants? I would expect that we would do something with them, like validate them against the capabilities the dev specified when creating the key, and also to recognise that some capabilities (eg. writeBucketRetentions) are present in the response but cannot be specified in the request.

But I don't want to do any of that, I'd rather just remove the consts and let the dev figure it out from the backblaze documentation.

* @throws GuzzleException If the request fails.
* @throws B2Exception If the B2 server replies with an error.
*
* @return Key
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@return Key[]

$keys[] = new Key($key['applicationKeyId'], $key['keyName'], null, $key['capabilities'], $key['bucketId'], $key['namePrefix'], $key['expirationTimestamp']);
}

return $keys;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you want to go a step further and use $response['nextApplicationKeyId'] to continually call the API to list keys beyond the first page, similar to how listFiles works with its while (true) loop?

protected $capabilities;
protected $bucketId;
protected $namePrefix;
protected $expirationTimestamp;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is also $options returned by the create, list and delete key api calls, please include it in the same way as these.

*/
public function __construct($id, $name, $type)
public function __construct($id, $name, $type, $options, $corsRules)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

While you're adding the extra fields you can include the remaining ones, bucketInfo, defaultFileLockConfiguration, lifecycleRules, revision

*
* @return Key
*/
public function deleteKey(array $options)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would you mind adding a test for each of the new functions?

public function listKeys(array $options = [])
{
$request = [
'accountId' => $this->accountId,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extra spacing here which isn't picked up by the linter for some reason, please fix it up while you're at it.

@mlambley
Copy link
Member

mlambley commented Mar 3, 2021

Hi @macbookandrew please let me know which of these changes you wish to do yourself and which you wish for me to do. Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants