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

transport: Introduce NewHTTPTransportOptions #1168

Merged
merged 8 commits into from
Jan 11, 2022
Merged

transport: Introduce NewHTTPTransportOptions #1168

merged 8 commits into from
Jan 11, 2022

Conversation

marclop
Copy link
Contributor

@marclop marclop commented Dec 8, 2021

Description

Introduces a new transport.NewHTTPTransportOptions() function which
accepts an Options struct to be able to customize the httptransport
without using environment variables

Introduces a new `transport.NewHTTPTransportOptions()` function which
accepts an Options struct to be able to customize the `httptransport`
without using environment variables

Signed-off-by: Marc Lopez Rubio <[email protected]>
@marclop marclop added enhancement New feature or request v8.0.0 labels Dec 8, 2021
@apmmachine
Copy link
Contributor

apmmachine commented Dec 8, 2021

💚 Build Succeeded

the below badges are clickable and redirect to their specific view in the CI or DOCS
Pipeline View Test View Changes Artifacts preview preview

Expand to view the summary

Build stats

  • Start Time: 2022-01-10T10:52:25.671+0000

  • Duration: 33 min 41 sec

  • Commit: 01e0471

Test stats 🧪

Test Results
Failed 0
Passed 12103
Skipped 279
Total 12382

🤖 GitHub comments

To re-run your PR in the CI, just comment with:

  • /test : Re-trigger the build.

  • run benchmark tests : Run the benchmark test.

  • run elasticsearch-ci/docs : Re-trigger the docs validation. (use unformatted text in the comment!)

@marclop marclop requested a review from a team January 4, 2022 07:22
@marclop marclop marked this pull request as ready for review January 4, 2022 07:22
transport/http.go Outdated Show resolved Hide resolved
transport/http.go Outdated Show resolved Hide resolved
transport/http.go Show resolved Hide resolved
}
t.SetServerURL(serverURLs...)
t.SetServerURL(opts.ServerURLs...)
Copy link
Member

Choose a reason for hiding this comment

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

IIANM this will panic if HTTPTransportOptions.ServerURLs is empty. This should probably also return an error.

Maybe worth adding a Validate method, like:

func (o HTTPTransportOptions) Validate() error {
    if len(o.ServerURLs) == 0 {
        // ...
    }
    if len(o.ServerTimeout) < 0 {
        // ...
    }
    return nil
}

Copy link
Contributor

@simitt simitt left a comment

Choose a reason for hiding this comment

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

In my opinion NewHTTPTransport and NewHTTPTransportOptions should deliever the same default values for non-customized options, so also ensure that the same default values for ServerURLs and ServerTimeout are set.
Can you please add a test for when using NewHTTPTransportOptions?

transport/http.go Show resolved Hide resolved
Signed-off-by: Marc Lopez Rubio <[email protected]>
Signed-off-by: Marc Lopez Rubio <[email protected]>
Copy link
Member

@axw axw left a comment

Choose a reason for hiding this comment

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

LGTM, just a couple of minor things

transport/http.go Show resolved Hide resolved
transport/http_test.go Outdated Show resolved Hide resolved
@marclop marclop requested a review from axw January 10, 2022 09:56
@marclop marclop merged commit 06e3549 into elastic:master Jan 11, 2022
@marclop marclop deleted the f/add-transport.NewHTTPTransportOptions branch January 11, 2022 05:13
@simitt simitt added this to the 2.0 milestone Feb 9, 2022
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.

4 participants