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(influxdb2/client): Send Content-Type headers #30

Merged
merged 2 commits into from
Mar 6, 2020
Merged

fix(influxdb2/client): Send Content-Type headers #30

merged 2 commits into from
Mar 6, 2020

Conversation

brettbuddin
Copy link
Contributor

@brettbuddin brettbuddin commented Mar 6, 2020

We recently started requiring Content-Type headers in InfluxDB 2.x API. This ensures the client is sending the appropriate headers.

  • CHANGELOG.md updated
  • Rebased/mergeable
  • Tests pass
  • Sign CLA (if not already signed)

@@ -24,6 +24,8 @@ class DefaultApi
DEFAULT_TIMEOUT = 10
DEFAULT_REDIRECT_COUNT = 10

HEADER_CONTENT_TYPE = 'Content-Type'.freeze

Choose a reason for hiding this comment

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

nice 😎

_post(payload, uri, headers: headers.merge({HEADER_CONTENT_TYPE => 'text/plain'}))
end

def _post(payload, uri, limit: @max_redirect_count, headers: {})

Choose a reason for hiding this comment

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

what was your rationale behind creating two separate methods, _post_json and _post_text which are identical aside from content-type? keeping the original _post method and exposing a content_type named argument w/ a reasonable default (e.g. application/json) might also be worth considering? (would also provide increased flexibility in allowing _post consumers to provide different content-type headers alongside their payloads if that's ever deemed beneficial)

(i guess, on second thought, not introducing another argument dependency in favor of encapsulating the content-type-setting logic in this class is probably more robust but still curious to hear your thoughts)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The underlying _post method is HTTP-centric so it warranted a higher-level headers concept if we are to be introducing any additional headers. However, this introduced a minor inconvenience at the JSON-based call sites so I just added some helpers.

Copy link

@reefloretto reefloretto left a comment

Choose a reason for hiding this comment

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

ship ship ship 🚢

@bednar bednar merged commit f010b74 into influxdata:master Mar 6, 2020
@reefloretto
Copy link

@bednar eta on when 1.2 will be released?

@bednar
Copy link
Contributor

bednar commented Mar 6, 2020

@reefloretto We will release clients at next friday, but if it is necessary We could release it early.

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.

3 participants