Skip to content

transport: Introduce NewHTTPTransportOptions#1168

Merged
marclop merged 8 commits intoelastic:masterfrom
marclop:f/add-transport.NewHTTPTransportOptions
Jan 11, 2022
Merged

transport: Introduce NewHTTPTransportOptions#1168
marclop merged 8 commits intoelastic:masterfrom
marclop:f/add-transport.NewHTTPTransportOptions

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 <marc5.12@outlook.com>
@marclop marclop added enhancement New feature or request v8.0.0 labels Dec 8, 2021
@ghost
Copy link

ghost 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
t.SetSecretToken(opts.SecretToken)
}
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?

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
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

Signed-off-by: Marc Lopez Rubio <marc5.12@outlook.com>
@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

3 participants