Skip to content

fix: cookie handling in normal requests and signout #229

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Merged
merged 6 commits into from
Aug 16, 2021
Merged

fix: cookie handling in normal requests and signout #229

merged 6 commits into from
Aug 16, 2021

Conversation

rs-blade
Copy link
Contributor

@rs-blade rs-blade commented Aug 14, 2021

Closes #

Proposed Changes

I changed the way the session cookie is set in the request because the way it was done before didn't work (the "Cookie" header is overwritten somewhere).

I also added the session cookie to the signout-Request as it was missing there.

Checklist

[x] Rebased/mergeable
[x] dotnet test completes successfully
[x] Commit messages are in semantic format
[x] Sign CLA (if not already signed)

@rs-blade rs-blade changed the title Fixed cookie handling in normal requests and add session cookie to signout fix: cookie handling in normal requests and signout Aug 14, 2021
@rs-blade
Copy link
Contributor Author

I don't know how the failing test can be related to my change but nevertheless I try to fix it. (I first have to find out how to configure a local InfluxDB so the tests can use it!)

@rs-blade
Copy link
Contributor Author

Hm. That test runs locally! So I would assume it's something not in my scope. So I ignore it.
Can anyone re-run the jobs please (I don't have the permissions to do so) maybe the failure disappears :-)

Copy link
Contributor

@bednar bednar left a comment

Choose a reason for hiding this comment

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

Hi @ruediger-stevens,

thanksfor your PR 👍. There is a few requirements that should be satisfy before we accept the PR.

  1. I've fixed the failing test in master branch. Please, rebase your sources with origin/master
  2. Please update README.md

Regards

@rs-blade
Copy link
Contributor Author

@bednar : If you really meant README.md then I don't know what I should update. I assume you really meant CHANGELOG,md, aren't you?

@bednar
Copy link
Contributor

bednar commented Aug 16, 2021

@ruediger-stevens, Yeah, sorry. Definitely the CHANGELOG.md.

@rs-blade
Copy link
Contributor Author

CHANGELOG.md is updated and repo is rebased (hopefully...never done that before :-) )

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2021

Codecov Report

Merging #229 (fc95a57) into master (8ae7ac3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master     #229   +/-   ##
=======================================
  Coverage   85.19%   85.20%           
=======================================
  Files          71       71           
  Lines        6235     6237    +2     
=======================================
+ Hits         5312     5314    +2     
  Misses        923      923           
Impacted Files Coverage Δ
Client/Internal/ApiClient.cs 94.04% <100.00%> (+0.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ae7ac3...fc95a57. Read the comment docs.

@bednar bednar merged commit 645a34a into influxdata:master Aug 16, 2021
@bednar bednar added this to the 2.1.0 milestone Aug 16, 2021
@bednar
Copy link
Contributor

bednar commented Aug 16, 2021

@ruediger-stevens, Thanks again for your PR. It is merged into master.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants