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

Fix URL override not being used for API calls #126

Merged

Conversation

agrare
Copy link
Member

@agrare agrare commented Dec 9, 2024

The openapi generated ApiClient/Configuration classes have a way to to pass scheme, host, and base_path but they are ignored and Configuration#server_settings is used unless server_index is set to nil

@agrare
Copy link
Member Author

agrare commented Dec 9, 2024

cc @Fryguy

There appear to be two ways to do this in an OpenAPI generated gem.

You can either provide scheme, host, and base_path with server_index=nil (https://github.com/xlab-si/intersight-sdk-ruby/blob/main/lib/intersight_client/configuration.rb#L193-L194)
Or you can provide server_variables that override the variable parameters in the servers section of the openapi spec (https://github.com/xlab-si/intersight-sdk-ruby/blob/main/lib/intersight_client/configuration.rb#L196-L213)

The latter seems like the "nicer" way to do it, only downside seems to be that the scheme can't be overridden.

@agrare agrare force-pushed the fix_api_client_scheme_host_not_being_used branch from 3152bde to 2742924 Compare December 9, 2024 19:33
@agrare
Copy link
Member Author

agrare commented Dec 9, 2024

We had some existing specs which tested a "http://intersight.localdomain:8080" case so I went with the server_index=nil route

@agrare agrare added bug Something isn't working quinteros/yes? labels Dec 9, 2024
verify_ssl = OpenSSL::SSL::VERIFY_PEER if verify_ssl.nil?
verify_ssl = verify_ssl == OpenSSL::SSL::VERIFY_PEER

IntersightClient::ApiClient.new(
IntersightClient::Configuration.new do |config|
config.scheme = scheme
config.host = host
if url && url != "https://intersight.com"
Copy link
Member

@Fryguy Fryguy Dec 9, 2024

Choose a reason for hiding this comment

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

String comparison of the URL has me slightly concerned, only because someone could also put "https://intersight.com/" (trailing slash), and that should technically go through here too - I wonder if there's way to parse the URL object anyway, then check if url.hostname == "intersight.com"?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah a bunch of edge cases here, if someone sets url=http://intersight.com:1234/some-other-base-path I'm not sure that we would want to default to https://intersight.com which is what would happen there.
Can certainly check each component of the URI though the code is going to get a little gross though :D

Copy link
Member Author

@agrare agrare Dec 9, 2024

Choose a reason for hiding this comment

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

Maybe URI::Generic#== can rescue us here

>> URI.parse('https://intersight.com') == URI.parse('https://intersight.com:443/') 
=> true

@agrare agrare force-pushed the fix_api_client_scheme_host_not_being_used branch from 2742924 to e122027 Compare December 9, 2024 20:23
The openapi generated `ApiClient`/`Configuration` classes have a way to
to pass scheme, host, and base_path but `Configuration#server_settings`
is used unless `Configuration#server_index` is set to `nil`.
@agrare agrare force-pushed the fix_api_client_scheme_host_not_being_used branch from e122027 to 6d8e4f0 Compare December 9, 2024 20:27
@Fryguy Fryguy merged commit d25a318 into ManageIQ:master Dec 9, 2024
3 of 4 checks passed
@Fryguy Fryguy self-assigned this Dec 9, 2024
@agrare agrare deleted the fix_api_client_scheme_host_not_being_used branch December 9, 2024 21:58
@Fryguy
Copy link
Member

Fryguy commented Jan 7, 2025

Backported to radjabov in commit ddb024d.

commit ddb024d34e5b6c5f4a99973077d26c12cab06d81
Author: Jason Frey <[email protected]>
Date:   Mon Dec 9 16:50:59 2024 -0500

    Merge pull request #126 from agrare/fix_api_client_scheme_host_not_being_used
    
    Fix URL override not being used for API calls
    
    (cherry picked from commit d25a318db5f004b34c5fb2882a305446f5352fae)

Fryguy added a commit that referenced this pull request Jan 7, 2025
…ing_used

Fix URL override not being used for API calls

(cherry picked from commit d25a318)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants