Application credentials support #543
Conversation
|
@geemus any chance of getting this merged? |
| class Real | ||
| def create_application_credentials(credential = {}) | ||
| user_id = credential.delete('user_id') || credential.delete(:user_id) | ||
| puts Fog::JSON.encode(:application_credential => credential) |
There was a problem hiding this comment.
I suspect this was unintentionally left behind from testing/debugging.
| puts Fog::JSON.encode(:application_credential => credential) |
| end | ||
| end | ||
|
|
||
| class Mock |
There was a problem hiding this comment.
For consistency with other parts of fog, could you add a method to each of the mock versions that has the same signature, and whose body is:
raise Fog::Errors::MockNotImplemented
That way it helps ensure that the behavior is obvious/consistent, as opposed to potentially NoMethod errors or even silently failing if the method is defined without a body as below.
| class V3 | ||
| class Real | ||
| def get_application_credentials(id, user_id) | ||
| puts "get app creds id: #{id}, from user #{user_id}" |
There was a problem hiding this comment.
Looks like another leftover.
| puts "get app creds id: #{id}, from user #{user_id}" |
geemus
left a comment
geemus
left a comment
There was a problem hiding this comment.
Hey, thanks for the additions and apologies for my delay in reviewing.
Could you please remove the puts statements and refine the unimplemented mock stuff? I think it should be good to merge once those are adjusted.
Thanks!
geemus
left a comment
geemus
left a comment
There was a problem hiding this comment.
It would also be good to get some tests for this too, now that I think about it. I'll bring this in for now, but we should probably follow up with tests soon if possible to help keep it working. It's just a bit hard to have good tests without mocks, and that's a whole other chunk of work.
|
Plus the main test suite has been failing for some time now, so also not ideal. |
|
@andrejcermak did you need a release with this? |
|
Yes, I intend to use this in other project. I think I can have a look at the tests too. |
|
@andrejcermak released in v1.1.5 |
2 participants
Hi, I added the application credential support that was missing in this library.