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

feat: init api server configs with path of host  #511

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

DonCamillo72
Copy link

@DonCamillo72 DonCamillo72 commented May 11, 2023

Reason:
If the influxdb server runs behind a reverse proxy with url e.g. "https://anyhost/influxdb" the influx-cli could not access the url because the host base path of the api url is always empty.

Idea:
The information of the base can be passed in the cli with the host parameter like this:

influx backup --host "https://anyhost/influxdb"

Implementation:
The base path is available in the parameter params.Host.Path and could be forwarded to the OperationServers of the generated client API

@albrecht-j
Copy link

Hi @jeffreyssmith2nd i saw you created the last release. Maybe you can take a look at this PR?

Thank you :)

@albrecht-j
Copy link

Hi @srebhan can you support us here? I think it is less effort then the last one ;-)

If not, whom can we contact here?
Best Regards
Johannes

@srebhan
Copy link

srebhan commented Aug 2, 2023

@albrecht-j sorry but this is not my field of expertise. ;-)

@jeffreyssmith2nd
Copy link
Contributor

Hey @albrecht-j, I'm the right person for this. My apologies, I just lost track of this PR. I will get it reviewed before the next release.

@albrecht-j
Copy link

Hey @jeffreyssmith2nd
any updates to this PR? If we need to change something please let us know :)

@DonCamillo72
Copy link
Author

Hi @jeffreyssmith2nd,
have you already reviewed the PR?
Please let us know if something need to be changed.
Thank you!

@albrecht-j
Copy link

Hi @jeffreyssmith2nd,

do you have a schedule for the next release?

@tisonkun
Copy link

Seems a function duplicatie to #530.

But the InfluxDB team doesn't care. I can have my fork but perhaps switch to some SDK.

I remember Python SDK v1 has such path. Not sure if v2 has.

// initialize api server configurations with path of host url
apiConfig.Servers = ServerConfigurations{{URL: params.Host.Path}}
for key := range apiConfig.OperationServers {
apiConfig.OperationServers[key] = ServerConfigurations{{URL: params.Host.Path}}
Copy link
Member

Choose a reason for hiding this comment

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

Overall, this looks relatively straightforward and like it will address the issue.

My one reservation is that it completely overrides the existing apiConfig.OperationServers[key] value. We don't lose any valuable information today (just a non-useful description), but I'm concerned this could introduce subtle issues in the future if more valuable information is already populated in the ServerConfigurations.

Would it be possible to set the URL value without overwriting everything else?

Choose a reason for hiding this comment

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

@gwossum Just to handle the path part, you can take a look at #530 which do a light prepend on constructing the final request.

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.

None yet

6 participants