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

apply given API service client options #418

Merged
merged 1 commit into from
Sep 22, 2018

Conversation

AlexanderZagaynov
Copy link
Contributor

That feature was broken with Google's gem update and
this PR: 5ec9a15#diff-c6b2ed334d1938d66b4e168f255abbb7L140

Now it's back.

# @param [Hash] google_client_options Service client options to apply
# @param [Hash] _ Rest of the options (for convenience, ignored)
# @return [void]
def apply_client_options(service, google_client_options: nil, **_)

Choose a reason for hiding this comment

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

Naming/UncommunicativeMethodParamName: Method parameter must be at least 3 characters long.

@Temikus
Copy link
Member

Temikus commented Sep 17, 2018

@AlexanderZagaynov Thank you very much for the contribution!

LGTM overall. If integration tests pass I'll try to merge this with 1.8.0 I'm planning to release anyway.

@AlexanderZagaynov
Copy link
Contributor Author

@Temikus About testing - can you please give me a hint where to put those tests?
I found nothing similar in existing code.
As I understood, codecov want me to cover

def apply_client_options(service, google_client_options: nil, **_ignored)

But I have no idea about proper way to do that.

@Temikus
Copy link
Member

Temikus commented Sep 18, 2018

@AlexanderZagaynov I don't think we have a reasonable module to test Shared yet, so I'll write one.

Only thing I ask is - can you please give me a couple of examples of common use-cases? There are a lot of options so I want to be sure we're testing the right thing.

@AlexanderZagaynov
Copy link
Contributor Author

@Temikus we're going to use that in a way like:

conn = Fog::Compute::Google.new(google_client_options: { proxy_url: 'https://user:password@host:8080' })
conn.list_servers('')

it should give ArgumentError: unsupported proxy
without proxy_url it should simply try to connect (returns Invalid request in example above).
old PR was here: #154

@Temikus Temikus merged commit 597e0f7 into fog:master Sep 22, 2018
@Temikus
Copy link
Member

Temikus commented Sep 22, 2018

@AlexanderZagaynov Thanks for the contribution, I've added a small test for it. It's not ideal but should cover at least a part of the risk surface.

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

Successfully merging this pull request may close these issues.

3 participants