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(core): don't check if its https if its not a browser #524

Merged
merged 1 commit into from
Apr 26, 2021

Conversation

jarlah
Copy link
Contributor

@jarlah jarlah commented Apr 21, 2021

In air-api in application-services I have thousand of thousands of requests that push "You should use SSL (https) when you login with OAuth since CDF only allows redirecting back to an HTTPS site" to the error logs in Stackdriver in production. It's pretty much a useless warning (that sadly is picked up with severity ERROR by Stackdriver). But the problem is, why should it be there? When there is no browser? So, the isBrowser was added to avoid "null pointer exceptions". This change is making sure its never called unless its a browser. specifically where it tries to log the statement above.

I assume the function isUsingSSL is used other places, so I don't touch it.

@jarlah jarlah requested a review from a team as a code owner April 21, 2021 19:33
@jarlah jarlah requested review from a team and vegardok April 21, 2021 19:33
@codecov
Copy link

codecov bot commented Apr 21, 2021

Codecov Report

Merging #524 (cdecad8) into master (f9fe5c0) will not change coverage.
The diff coverage is 100.00%.

❗ Current head cdecad8 differs from pull request most recent head d988e7f. Consider uploading reports for the commit d988e7f to get more accurate results

@@           Coverage Diff           @@
##           master     #524   +/-   ##
=======================================
  Coverage   79.23%   79.23%           
=======================================
  Files          82       82           
  Lines        2605     2605           
  Branches      377      377           
=======================================
  Hits         2064     2064           
  Misses        524      524           
  Partials       17       17           
Impacted Files Coverage Δ
packages/core/src/baseCogniteClient.ts 92.00% <100.00%> (ø)

Copy link
Collaborator

@polomani polomani left a comment

Choose a reason for hiding this comment

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

LGTM

@vegardok
Copy link
Contributor

you have to add [release] to the PR for it to be released to the current major version

@jarlah jarlah changed the title fix(core): don't check if its https if its not a browser release(core): don't check if its https if its not a browser Apr 26, 2021
@jarlah jarlah changed the title release(core): don't check if its https if its not a browser feat(core): don't check if its https if its not a browser Apr 26, 2021
@jarlah jarlah changed the title feat(core): don't check if its https if its not a browser feat(core): don't check if its https if its not a browser [release] Apr 26, 2021
@jarlah jarlah changed the title feat(core): don't check if its https if its not a browser [release] feat(core): don't check if its https if its not a browser Apr 26, 2021
@jarlah jarlah merged commit 4ed1528 into master Apr 26, 2021
@jarlah jarlah deleted the fixForServerApplications branch April 26, 2021 14:14
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