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 createKey method #56

Closed
wants to merge 10 commits into from
Closed

Conversation

macbookandrew
Copy link

No description provided.

@mhetreramesh
Copy link
Member

Hi @macbookandrew,

Can you describe your change for better understanding?

@macbookandrew
Copy link
Author

Sure—sorry, I should have included that to start with. There are two changes in this PR:

  1. Add a createKey method to create Application Keys
    • Should I add some tests for this?
  2. Add more detail to the bucket API calls so consuming code can determine whether the bucket is S3-compatible and get other bucket options

@ersin-comet
Copy link

Hello @mhetreramesh @macbookandrew

This is useful for my current work, thanks for taking the time to work on this!

I have been able to integrate and use this new method, but noticed there is a minor issue with the Key class.

When I call $client->create_key($options) I expect that the created key instance has the correct value returned for getName() and getSecret() methods, however it appears that these keys are swapped around.

What can I do to get this merged upstream?

@macbookandrew
Copy link
Author

@ersin-comet Thanks.

however it appears that these keys are swapped around.

You’re right…fixed in 6edc18c

I also added listKeys() and deleteKey() methods.

@mlambley mlambley closed this Feb 3, 2021
@mlambley mlambley deleted the branch gliterd:develop February 3, 2021 10:20
@macbookandrew
Copy link
Author

@mlambley Just curious—did I miss something you wanted in order to merge this? Or would you prefer to keep application keys management out of this package?

@mlambley
Copy link
Member

mlambley commented Feb 4, 2021

@macbookandrew I didn't mean to close this PR sorry. You will have to recreate it.

@mlambley
Copy link
Member

mlambley commented Feb 4, 2021

@macbookandrew Then tag me in the new PR and I'll review it.

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.

4 participants